Repository navigation
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Budget truncation can cause finalization to enable WarpSpeed for more than three eligible collectives, bypassing the correctness guard and conflicting with channel packing, while helper-only tests miss this enqueue-level failure.
2 open issues
What changed in this PR
This PR narrows RCCL’s three-collective plan cap to WarpSpeed-eligible prefixes, aiming to restore budget-based coalescing for ordinary collectives without reverting earlier graph-capture optimizations.
Changes:
- Moves the cap after prefix estimation and recalculates channel budgets when applied.
- Adds a shared helper using plan-wide WarpSpeed eligibility.
- Adds helper-level coverage for eligible, mixed, Tree, and 58-task ordinary prefixes, with expanded fake argument tracking.
| File | Description |
|---|---|
projects/rccl/test/RcclWrapTests.cpp |
Adds eligibility and cap-helper tests using real task lists. |
projects/rccl/test/host/scheduler-test.cc |
Adds helper microtests for prefix counts and ordinary work. |
projects/rccl/test/host/fakes/rccl_wrap_fakes.h |
Declares captured eligibility-call arguments. |
projects/rccl/test/host/fakes/rccl_wrap_fakes.cc |
Implements prefix-aware eligibility fakes and resets. |
projects/rccl/src/include/rccl_common.h |
Adds the WarpSpeed-specific cap predicate. |
projects/rccl/src/enqueue/enqueue.cc |
Replaces the unconditional cap with an estimated-prefix eligibility check. |
Review effort: Balanced
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ARGUS
Findings
5 more finding(s), not posted inline
projects/rccl/src/enqueue/enqueue.cc:939(Low) The WarpSpeed re-budget loop (938-945) copies the kind/trafficBytes/nChannels accumulation from 902-915; share one small lambda between both loops.projects/rccl/src/enqueue/enqueue.cc:933(Low) The WarpSpeed plan limit is the literal 3 here and innPlanColls > 3at rccl_common.h:518; name it once (e.g. a constexpr in rccl_common.h) and use it in both places.projects/rccl/test/host/enqueue-test.cc:3284(Low) No test fails if the!p2pMayFollowgate on the cap (enqueue.cc:931) is deleted, because the deferred-P2P scene has only 3 tasks; use 4 eligible tasks.projects/rccl/test/host/enqueue-test.cc:3300(Low) WarpSpeedPlanner_LaunchGridUsesStoredPlanMode stack-allocates a multi-MB ncclComm (MAXCHANNELS 512 under ENABLE_WARP_SPEED); heap-allocate it like BatchPlanComm does. Same at scheduler-test.cc:242 and 260.projects/rccl/src/include/rccl_common.h:510(Nit) The 2-arg rcclWarpSpeedSupported(comm, plan) has no callers left now that finishPlan and launch read plan->warpSpeedPlan; drop it here, at rccl_wrap.cc:2865 and its fake at rccl_wrap_fakes.cc:152.
Bottom line: Worth settling before approval: p2pMayFollow at enqueue.cc:925 reads rank-local nTasksP2p, so ranks with and without a send/recv can pack the same collectives in different WarpSpeed modes.
Comment @argus review to re-run, or @argus re-review after pushing.
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: the fake rcclShouldCapGroupedPlan at rccl_wrap_fakes.cc:160 already diverges from production (strcmp vs IsArchMatch), and mixed [E,E,E,O] groups now lose WarpSpeed at rccl_wrap.cc:2868.
Comment @argus review to re-run, or @argus re-review after pushing.
Contributor
pr-rocm-systems-14832-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 189 tests skipped. pr-rocm-systems-14832-oci-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-14832-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The change lacks launch-level tests proving mixed WarpSpeed and asymmetric P2P work is correctly deferred and drained, and repeated estimation of capped task prefixes introduces quadratic overhead.
3 open issues
1 resolved since last review
Review effort: Balanced
Contributor
pr-rocm-systems-14832-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-14832-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: whether the gfx950 grouped-plan cap can safely drop to single-node only (warp_speed_plan.h:22), since the corruption repro was never run across nodes.
Comment @argus review to re-run, or @argus re-review after pushing.
Contributor
pr-rocm-systems-14832-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-14832-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Preserve the broad single-node gfx950 safety bound while restoring budget-based coalescing for ordinary multi-node plans. Store the selected plan mode through packing, kernel arguments, and launch geometry, and defer rank-local P2P from WarpSpeed plans. Co-authored-by: ilia.kosarev <ilia.kosarev@amd.com>
Exercise conservative gfx950 capping, multi-node coalescing, stored plan mode, budget truncation, asymmetric P2P deferral, linear estimation, and bounded topology fixtures in macro-on/off host tests. Co-authored-by: ilia.kosarev <ilia.kosarev@amd.com>
cursor
Bot
force-pushed
the
cursor/warp-speed-plan-gate-0872
branch
from
November 4, 2026 18:24
c992026 to
de2123d
Compare
Contributor
pr-rocm-systems-14832-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-14832-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Changes to distributed GPU plan partitioning, launch geometry, and P2P ordering require maintainer confirmation of final-revision gfx950 and multi-node correctness results, since host fakes cannot establish device-side safety.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
isaki001
approved these changes
Nov 5, 2026
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Changes to the GPU-corruption safeguard, distributed partitioning, and launch geometry require maintainer review of the reported gfx950 and multi-node validation because host fakes cannot verify device execution or synchronization.
No open issues 🎉
Review effort: Balanced
abouteiller
approved these changes
Nov 6, 2026
abouteiller
left a comment
abouteiller
left a comment
Contributor
There was a problem hiding this comment.
Recommend getting a review from @amd-jiali as she introduced the gate initially
This branch has not been deployed
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
An RCCL-only planner guard from the NCCL 2.31 sync capped every collective plan at three tasks. It protected a single-node gfx950 grouped-AllReduce correctness case, but also split ordinary multi-node captured work, multiplying kernels and callbacks and reducing DeepSeek/SGLang throughput.
Technical Details
warpSpeedPlanonce and reuse it for channel packing, kernel arguments, and launch geometry. Rank-local P2P is deferred to a separate ordinary plan.Issue Tracking
JIRA ID: AICOMRCCL-3523
Test Plan
Host planner coverage exercises macro-on/off builds, single-node gfx950 mixed prefixes, multi-node ordinary groups, budget truncation, task conservation/order, stored launch mode, asymmetric P2P deferral, bounded estimation, and topology-buffer safety. Hardware validation covers the historical grouped case, a two-node correctness gate, a 58-task graph oracle, and the full DeepSeek workload.
Test Result
RcclWrapTests.cpphost/gfx950 object compilation passed.2 nodes / 1 edge); force-on remained protected.2/1nodes/edges at 301–302 µs versus base40/39at 2577–2582 µs; stock was2/1at 299 µs.1323.82 → 1763.73 tok/s(+33.23%), ITL −25.98%, E2E −25.26%; 128/128 requests, zero HSA faults or NIC error deltas.Submission Checklist