Make cuBLASLt descriptor wrappers move-safe - #3078
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdates cuBLASLt handle wrappers with explicit ChangescuBLASLt RAII Move Semantics
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
achirkin
left a comment
There was a problem hiding this comment.
Thanks for your contribution! The change looks reasonable. However, before we move forward, could you please tell what is the original bug/problem being addressed? These wrapper types are in the detail namespace, that is not supposed to be used outside of the raft implementation details.
| if (res != nullptr) { RAFT_CUBLAS_TRY_NO_THROW(cublasLtMatrixLayoutDestroy(res)); } | ||
| res = std::exchange(other.res, nullptr); | ||
| } |
There was a problem hiding this comment.
Rather than having multiple cublasLtMatrixLayoutDestroy uses, perhaps, a cleaner solution would be to use std::swap(this->res, other.res) and rely on the other one being correctly destroyed?
2b09036 to
98290ce
Compare
|
A small repro is: On main, the first expectation fails because the defaulted move leaves both objects holding the same handle. When they go out of scope, both destructors try to destroy it. |
|
/ok to test fa7aae3 |
|
/merge |
Picked up real upstream rapidsai/raft changes between 26.08.00 and 26.10.00 (VERSION bumped as a side effect of the last origin/tuned-builds merge): native row-major PCA (NVIDIA#3036), predictable raft::resources (NVIDIA#3052), an nvtx/host-mem-resource symbol export fix (NVIDIA#3083), orphaned select_k declaration removal (NVIDIA#3084), move-safe cuBLASLt descriptor wrappers (NVIDIA#3078), cuda::std adoption in LAP kernels and stats minmax (NVIDIA#3094, NVIDIA#3095), and two real cuBLASLt/GEMM correctness fixes for CUDA 13.6 (NVIDIA#3098, NVIDIA#3100). No formal changelog exists yet for 26.08/26.10 (CHANGELOG.md's newest entry is still 26.06.00) -- compiled from git log between the two version points instead. Rebuilt, repackaged, and republished as v26.10-gb10-cu133 (https://github.com/zbrad/raft/releases/tag/v26.10-gb10-cu133), superseding v26.8-gb10-cu133 (deleted). Assisted-by: Claude Sonnet 5
Summary
Why
These wrappers own raw
cublasLt*descriptors and destroy them in their destructors, but their move constructor and move assignment operator were defaulted. That meant moving a wrapper copied the raw handle into the destination without clearing the source, so both objects believed they owned the same descriptor.Impact
This closes a double-destroy hazard in any path that moves or move-assigns these wrappers, including code that caches or returns compound matmul descriptor objects.
Root cause
The wrapper types were acting like RAII owners while still using raw-pointer default move semantics.
Validation
git diff --checkPARALLEL_LEVEL=8 ./build.sh tests -n --limit-tests=LINALG_TESTnvccis not installed and CMake cannot resolveCUDAToolkit_ROOT