Use l-values to initialize RMM constructors - #3123
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 (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes update SPDX attribution and revise device scalar construction in CUDA tests and the MST solver to use named initialization values or default construction before existing zero initialization. ChangesDevice scalar initialization
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This is a localized bug fix to constructor initialization, and no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 5 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
bdice
left a comment
There was a problem hiding this comment.
LGTM. I shared one alternative pattern that you can adopt if you wish. Up to you.
| T zero = 0; | ||
| rmm::device_scalar<T> ref(zero, resource::get_cuda_stream(handle)); |
There was a problem hiding this comment.
This is a special case where you can use a different pattern if you want.
| T zero = 0; | |
| rmm::device_scalar<T> ref(zero, resource::get_cuda_stream(handle)); | |
| rmm::device_scalar<T> ref(resource::get_cuda_stream(handle)); | |
| ref.set_value_to_zero_async(resource::get_cuda_stream(handle)); |
|
|
||
| // Test out both the device and host api's | ||
| rmm::device_scalar<T> out(0, resource::get_cuda_stream(handle)); | ||
| rmm::device_scalar<T> out(zero, resource::get_cuda_stream(handle)); |
There was a problem hiding this comment.
If you adopt the above, adopt this too:
| rmm::device_scalar<T> out(zero, resource::get_cuda_stream(handle)); | |
| rmm::device_scalar<T> out(resource::get_cuda_stream(handle)); | |
| out.set_value_to_zero_async(resource::get_cuda_stream(handle)); |
|
/merge |
|
|
||
| rmm::device_scalar<int> counter{0, s}; | ||
| int zero = 0; | ||
| rmm::device_scalar<int> counter{zero, s}; |
There was a problem hiding this comment.
These changes fix the compile error, but, from a cursory look (unless raft::launch_kernel synchronises the stream) do not fix the latent bug. The stream still needs to be synced after the copy is queued up
There was a problem hiding this comment.
Device to Host memory is synchronous https://docs.nvidia.com/cuda/cuda-runtime-api/api-sync-behavior.html#api-sync-behavior__memcpy-async
But yeah, the stream sync should probably be explicit so that we don't rely on hidden mechanisms.
rapidsai/rmm#2527 disallowed using r-values for aysnchronous constructors, so this PR fixes an existing bug.