Mitigate cuBLASLt 13.6 GEMM bug for inputs greater than 2^31 - #3098
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesGEMM workaround and regression coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cpp/tests/linalg/gemm_large.cpp (2)
74-84: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate an interior result too.
Checking only endpoints can miss a partial/corrupt result in the large output. Copy and assert at least one representative interior element, such as
samples / 2.As per path instructions, “Validate numerical correctness in the test beyond just edge elements as needed to avoid false positives.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/linalg/gemm_large.cpp` around lines 74 - 84, Extend the output validation in the large GEMM test to copy and assert a representative interior element, such as index samples / 2, in addition to the existing endpoint checks. Update the output_edges storage and asynchronous cudaMemcpyAsync calls while preserving synchronization and the existing zero-value assertions.Source: Path instructions
46-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse RAFT owning mdarrays for the test buffers.
These are straightforward owned device buffers; replace
rmm::device_uvectorwith suitable RAFT device vector/mdarray types and pass their data handles to GEMM.As per coding guidelines, “Prefer
raftmdarray types for owning data overrmm::device_uvector… when an mdarray fits the use case.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/linalg/gemm_large.cpp` around lines 46 - 48, Replace the three rmm::device_uvector test buffers in the GEMM setup with suitable owning RAFT device vector/mdarray types, preserving their element counts and stream allocation. Update the GEMM calls to pass the RAFT containers’ data handles instead of relying on rmm::device_uvector interfaces.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/tests/linalg/gemm_large.cpp`:
- Around line 74-84: Extend the output validation in the large GEMM test to copy
and assert a representative interior element, such as index samples / 2, in
addition to the existing endpoint checks. Update the output_edges storage and
asynchronous cudaMemcpyAsync calls while preserving synchronization and the
existing zero-value assertions.
- Around line 46-48: Replace the three rmm::device_uvector test buffers in the
GEMM setup with suitable owning RAFT device vector/mdarray types, preserving
their element counts and stream allocation. Update the GEMM calls to pass the
RAFT containers’ data handles instead of relying on rmm::device_uvector
interfaces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 09766a0a-0585-481a-b532-56b3fb3e8258
📒 Files selected for processing (5)
build.shcpp/include/raft/linalg/detail/cublaslt_wrappers.hppcpp/tests/CMakeLists.txtcpp/tests/linalg/gemm_basic.cppcpp/tests/linalg/gemm_large.cpp
csadorf
left a comment
There was a problem hiding this comment.
Have you considered constraining our dependency?
| { | ||
| constexpr uint64_t max_safe_span = (uint64_t{1} << 31) - 1; | ||
| const auto a_columns = args.trans_a ? args.m : args.k; | ||
| return version == 130600 && args.lda != 0 && a_columns > max_safe_span / args.lda; |
There was a problem hiding this comment.
Did you test other versions? Are we confident that only this very specific version is affected? What about future versions?
There was a problem hiding this comment.
No, but the tests should help with that if it fails in a future version. If you prefer being defensive, then I can add a >= check instead?
There was a problem hiding this comment.
I think we might have to be defensive here and actually set 130600 as a lower bound unless we get some kind of assurance that this will be fixed in the next patch release.
| * cuBLASLt 13.6 may select algorithm 68 once A's physical span reaches 2^31 elements, but that | ||
| * algorithm fails during execution for FP32. Prefer the validated alternatives. | ||
| */ | ||
| inline auto needs_cublaslt_13_6_workaround(const matmul_key_t& args, std::size_t version) noexcept |
There was a problem hiding this comment.
I'd recommend checking whether the work-around is needed on all archs or only sm_120/sm_121.
There was a problem hiding this comment.
This will be a bit tough to verify this close to the release.
There was a problem hiding this comment.
Yes, but I think we should at least check sm_90 since restricting the algorithm might lead to a performance regression.
There was a problem hiding this comment.
Good point, let me skip the heuristic for non SM120/121 and CI tests should give us an indication for sm80 and sm90.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tests/linalg/gemm_large.cpp`:
- Around line 30-32: Update the test setup around the device vector allocations
for a, b, and c to query available GPU memory and skip the test when the
required allocation, including a suitable overhead margin, cannot be satisfied.
Ensure the skip occurs before constructing any of these vectors and reports that
insufficient free device memory makes the test unsupported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 22149fed-0883-4328-b9d7-95d09c8ab553
📒 Files selected for processing (2)
cpp/include/raft/linalg/detail/cublaslt_wrappers.hppcpp/tests/linalg/gemm_large.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tests/linalg/gemm_basic.cpp`:
- Around line 205-216: Add an EXPECT_FALSE assertion for device architecture
(12, 2) in the GemmCublasLt136WorkaroundArchitectures test, alongside the
existing SM12 cases, using the same at_boundary and affected_version inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b9fa7dc9-3460-4b7e-bfec-9c7b88be42f6
📒 Files selected for processing (2)
cpp/include/raft/linalg/detail/cublaslt_wrappers.hppcpp/tests/linalg/gemm_basic.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/include/raft/linalg/detail/cublaslt_wrappers.hpp
dantegd
left a comment
There was a problem hiding this comment.
One quick question, besides it lgtm
|
|
||
| constexpr int workaround_heuristic_results = 2; | ||
| std::array<cublasLtMatmulHeuristicResult_t, workaround_heuristic_results> heuristic_results{}; | ||
| const int requested_results = use_cublaslt_13_6_workaround ? workaround_heuristic_results : 1; |
There was a problem hiding this comment.
Is it guaranteed or documented that these affected GEMMs always have a usable non-68 algorithm in the top two heuristic results?
There was a problem hiding this comment.
No, but this only skips 68 if it's at the top of the heuristics. If nothing but 68 is returned we will have to fail.
|
/merge |
The fix in #3098 was incomplete, as according to the cublas team there is no guarantee that `algo13` will be returned along with `algo68` (the failing version for A matrix with > 2^31 elements). Also, the cublas team will patch this bug in version `13.6.1` so we will automatically receive the fix by guarding only against `13.6.0`.
Closes #3097