Repository navigation
perf(rccl): skip generic-kernel profiler scans and progress-counter syncs when disabled - #13402
Merged
rahulvaidya20 merged 2 commits intoNov 5, 2026
Merged
Conversation
rahulvaidya20
requested review from
a team
and
a balanced review from Copilot
October 13, 2026 20:17
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The flags preserve existing feature-enabled behavior while safely removing disabled-path overhead.
0 open findings
What changed in this PR
Optimizes RCCL generic kernels by bypassing profiler scans and progress synchronization when those features are disabled.
Changes:
- Adds kernel flags for profiler and progress-counter availability.
- Initializes flags per kernel plan.
- Uses flags to skip unnecessary device-side loops and barriers.
| File | Description |
|---|---|
projects/rccl/src/include/device.h |
Defines flags within existing argument-structure padding. |
projects/rccl/src/enqueue/enqueue.cc |
Initializes flags from profiler and counter state. |
projects/rccl/src/device/common.h |
Gates profiler scans and progress barriers. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
rahulvaidya20
force-pushed
the
users/rahulvaidya20/rccl-generic-kernel-devflags
branch
from
October 13, 2026 20:26
f79b6db to
74673ad
Compare
thomas-huber
approved these changes
Oct 13, 2026
pvallem
approved these changes
Oct 13, 2026
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: devFlags at enqueue.cc:356 has no assertion, and the zero-filled args buffer makes a dropped store indistinguishable from both features being off.
Comment @argus review to re-run, or @argus re-review after pushing.
Contributor
Author
|
Addressed the Argus finding on
No change to |
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Looks approval ready: the devFlags assertion gap is closed by the new cases. The remaining points are optional, the largest being whether the profiler bit should key on plan->hasProfilerOps rather than the process-global check at enqueue.cc:356.
Comment @argus review to re-run, or @argus re-review after pushing.
rahulvaidya20
force-pushed
the
users/rahulvaidya20/rccl-generic-kernel-devflags
branch
from
October 20, 2026 20:30
d42ec3a to
baac8c4
Compare
rahulvaidya20
force-pushed
the
users/rahulvaidya20/rccl-generic-kernel-devflags
branch
from
November 5, 2026 00:00
baac8c4 to
5cc5763
Compare
Contributor
pr-rocm-systems-13402-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
Contributor
pr-rocm-systems-13402-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
rahulvaidya20
deleted the
users/rahulvaidya20/rccl-generic-kernel-devflags
branch
November 5, 2026 16:27
Contributor
TheRock Submodule Bump ActivityNewest first
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The NCCL 2.32.3 sync added small-message latency to the generic kernel: Gather and Scatter are 0.3–0.35 µs slower per call on one node, and most other collectives 0.1–0.2 µs. The cost is per-batch work done by thread 0 while the rest of the block waits. Two parts of it run even when nothing reads the result: the profiler stamp loops and the progress-counter synchronization. This PR skips them when they are disabled.
Technical Details
projects/rccl/src/include/device.h: addRCCL_DEV_FLAG_PROFILERandRCCL_DEV_FLAG_PROGRESS, and a one-bytedevFlagsfield inncclDevKernelArgs. The field uses existing padding beforeworkMask, so the struct size and layout are unchanged.projects/rccl/src/enqueue/enqueue.cc(finishPlan): setPROFILERwhen a profiler plugin is loaded andPROGRESSwhen the progress-counter block is allocated, once per launch.projects/rccl/src/device/common.h:profiler(): skip the per-work-item stamp loops whenPROFILERis clear. Without a plugin every item'sprofilerEnabledbit is already false, so the loops wrote nothing; the signature is unchanged.ncclKernelMain: readPROGRESSonce from the kernel arguments and use it for the two progress-counter__syncthreads()guards, which used to test the shared-memory pointer.Issue Tracking
JIRA ID: AICOMRCCL-2916
JIRA ID: AICOMRCCL-2745
Test Plan
-n 100 -w 20 -N 5, with the correctness check on. Each comparison runs the pre-sync build (2.31.2), develop and this fix back to back on the same nodes, in rotated order over several rounds; numbers are mean time per call.Test Result
Submission Checklist