Skip to content

Use l-values to initialize RMM constructors - #3123

Merged
rapids-bot[bot] merged 2 commits into
NVIDIA:mainfrom
divyegala:rmm-lvalue-ctor
Aug 25, 2026
Merged

Use l-values to initialize RMM constructors#3123
rapids-bot[bot] merged 2 commits into
NVIDIA:mainfrom
divyegala:rmm-lvalue-ctor

Conversation

@divyegala

Copy link
Copy Markdown
Contributor

rapidsai/rmm#2527 disallowed using r-values for aysnchronous constructors, so this PR fixes an existing bug.

@divyegala divyegala self-assigned this Aug 25, 2026
@divyegala
divyegala requested a review from a team as a code owner August 25, 2026 18:27
@divyegala divyegala added bug Something isn't working non-breaking Non-breaking change labels Aug 25, 2026
@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ca7b537b-f6d4-4262-8306-e555d5a59685

📥 Commits

Reviewing files that changed from the base of the PR and between 6c933c2 and baa2e86.

📒 Files selected for processing (2)
  • cpp/include/raft/sparse/solver/detail/mst_solver_inl.cuh
  • cpp/tests/linalg/mean_squared_error.cu

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Walkthrough

Summary by CodeRabbit

  • Tests
    • Improved reliability and consistency of GPU validation across communication, linear algebra, atomic operations, sparse solver, and bit-counting scenarios.
    • Standardized initialization practices across numerical and device-based test coverage.
    • Updated test metadata to reflect current copyright coverage.
  • Maintenance
    • Refined internal test setup without changing application behavior, public interfaces, or end-user functionality.
    • Preserved existing asynchronous initialization and validation behavior while improving consistency.

Walkthrough

The 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.

Changes

Device scalar initialization

Layer / File(s) Summary
Test scalar initialization
cpp/include/raft/comms/detail/test.hpp, cpp/tests/linalg/dot.cu, cpp/tests/util/device_atomics.cu, cpp/tests/util/popc.cu, cpp/tests/linalg/mean_squared_error.cu
Tests now use named initialized values when constructing device scalars. SPDX notices were updated in the affected files.
MST solver scalar construction
cpp/include/raft/sparse/solver/detail/mst_solver_inl.cuh
Two device scalars now use default construction before their existing asynchronous zero initialization.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to baa2e

This is a localized bug fix to constructor initialization, and no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: achirkin

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: using l-values to initialize RMM constructors.
Description check ✅ Passed The description is related to the changeset and explains that the PR fixes r-value initialization for asynchronous RMM constructors.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

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)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@bdice bdice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I shared one alternative pattern that you can adopt if you wish. Up to you.

Comment thread cpp/tests/linalg/dot.cu
Comment on lines +66 to +67
T zero = 0;
rmm::device_scalar<T> ref(zero, resource::get_cuda_stream(handle));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a special case where you can use a different pattern if you want.

Suggested change
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));

Comment thread cpp/tests/linalg/dot.cu

// 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));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you adopt the above, adopt this too:

Suggested change
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));

@divyegala

Copy link
Copy Markdown
Contributor Author

/merge


rmm::device_scalar<int> counter{0, s};
int zero = 0;
rmm::device_scalar<int> counter{zero, s};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rapids-bot
rapids-bot Bot merged commit c0eaacd into NVIDIA:main Aug 25, 2026
152 of 160 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants