Repository navigation
fix(rccl): make CE AlltoAllv intra-batch sync count rank-invariant (AICOMRCCL-2474) - #12984
Merged
Merged
Conversation
…ICOMRCCL-2474) ncclCeAlltoAllv decided intraBatchSync from this rank's nonzero peer count and byte total, so with an uneven size matrix ranks issued a different number of ncclMemOpSync barriers and deadlocked. Decide from the full size matrix and sync per round of destination slots instead. Under graph capture, add one barrier when the round count would leave the total odd, so graph replays keep alternating the ready and complete flags.
…CCL-2474) Add rccl-UnitTestsMicroCeColl, which includes the hipified src/ce_coll.cc and runs ncclCeAlltoAllv once per simulated rank over a shared sparse size matrix. It checks that every rank issues the same number of ncclMemOpSync barriers and enqueues each destination slot in the same round, across the single-stream, multi-stream, batch-async, legacy-null-stream and graph-capture launch paths, and that graph capture keeps the barrier count even. The sparse cases fail against the previous ce_coll.cc and pass with the fix. New fake seams: hipMemcpyBatchAsync, ncclDevrGetLsaRankPtr, the CE profiler events, and an omit guard for the ncclCeFinalize stub.
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
pvallem
requested review from
AbandiGa,
alex-breslow-amd,
nusislam and
sdarisala
October 7, 2026 03:00
![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: Looks approval ready: the barrier-count change traces rank-invariant on every path, including freq=0 and an all-zero send row. The five Low findings are all test coverage and build hygiene.
Comment @argus review to re-run, or @argus re-review after pushing.
…CCL-2474) Pin the dense case to the absolute round-sync barrier count, check the copy-stream copy count on every multi-stream run, and keep the BatchAsync arm in the parameter list with a skip reason when the SDK lacks hipMemcpyBatchAsync.
…est (AICOMRCCL-2474) Bracket the standalone probe with STATIC_LIBRARY like the other probes in test/host/CMakeLists.txt, and state the real reason ce_coll.cc needs its own micro binary.
Contributor
There was a problem hiding this comment.
ARGUS
Findings
1 finding(s) that could not be posted as inline comments
projects/rccl/CHANGELOG.md:46(Low) No CHANGELOG entry for a user-visible CE AlltoAllv hang fix No CHANGELOG entry for this fix. A CE AlltoAllv hang at 16+ ranks with uneven send sizes is the "changes impact library users" case in the approval checklist at projects/rccl/.github/PULL_REQUEST_TEMPLATE.md:20-23. Could you add a bullet to the unreleased Resolved issues list that ends at projects/rccl/CHANGELOG.md:46, alongside the comparable hang fix at projects/rccl/CHANGELOG.md:137?
Bottom line: The fix itself looks approval ready. Two Low items are open: a CHANGELOG bullet, and a case with an even (nRanks-1)/freq so the capture parity branch is covered.
Comment @argus review to re-run, or @argus re-review after pushing.
Contributor
There was a problem hiding this comment.
ARGUS
Findings
2 finding(s) that could not be posted as inline comments
projects/rccl/test/host/MICROTEST_README.md:352(Low) fakes-map row for src/ce_coll.cc is stale now that a target compiles the real fileMICROTEST_README.md:352still mapssrc/ce_coll.cctofakes/ce_fakes.ccunqualified, butrccl-UnitTestsMicroCeCollcompiles the real file and drops that TU. Could that row take the(targets that do not compile the real file)qualifier thesrc/dev_runtime.ccrow atMICROTEST_README.md:354already carries?projects/rccl/CHANGELOG.md:47(Low) User-visible CE AlltoAllv hang fix is not recorded in the changelog The CE AlltoAllv hang at 16+ ranks with uneven send sizes is not recorded under### Resolved issuesfor 2.32.3 inCHANGELOG.md, where comparable user-visible fixes sit atCHANGELOG.md:43andCHANGELOG.md:47. Could it get a bullet there?
Bottom line: Looks approval ready: the barrier count recomputes rank-invariant for every shape checked, including nRanks not a multiple of freq. The three open items are Low doc and test coverage.
Comment @argus review to re-run, or @argus re-review after pushing.
nusislam
approved these changes
Oct 8, 2026
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Looks approval ready: the barrier count traces rank-invariant on every shape checked. The one gap worth closing is that no test gives a rank an all-zero send row, which is the shape the fix exists for.
Comment @argus review to re-run, or @argus re-review after pushing.
…CL-2474) Use the binary, source list, XML name and CTest category layout that the CE staging fix (ROCM-32044) introduces for the same src/ce_coll.cc unit, so the two branches build one rccl-UnitTestsMicroCe instead of two binaries. - rccl-UnitTestsMicroCeColl becomes rccl-UnitTestsMicroCe, built from TEST_MICRO_CE_SOURCE_FILES and placed after the Enqueue targets. - The suite registers in test_categories_micro.yaml; the per-binary test_categories_micro_cecoll.yaml is removed. - The CE profiler stubs move from nccl_stubs.cc to comm_fakes.cc, and the hipMemcpyBatchAsync wrapper sits next to hipStreamWriteValue32, matching where that branch defines them, so a merge cannot leave duplicate definitions. - MICROTEST_README.md: the bullet moves next to the Enqueue one, and the standalone build list gets back the closing backtick on GinHost.
…ync exits (AICOMRCCL-2474) Four CE AlltoAllv microtest cases, each run on all five launch paths: - One rank with an all-zero send row while every other row crosses both gates: the silent rank must issue the same barrier count and no copies. - 17 ranks at freq 8: an even (nRanks - 1) / freq, so graph capture needs no extra barrier, and a one-slot last round. - The first round's copy fails, and the first round barrier fails: both must return ncclUnhandledCudaError and stop at the barrier count they reached. Each new case is killed by its scratch mutation: the silent-rank case fails on develop's ce_coll.cc in all five arms, the 17-rank case fails under capture with the parity check removed, and the two failure cases fail in all five arms when the corresponding NCCLCHECKGOTO is dropped.
…CL-2474) Add a Resolved issues bullet for the CE ncclAlltoAllv hang, and qualify the src/ce_coll.cc row of the fakes map in MICROTEST_README.md now that rccl-UnitTestsMicroCe compiles the real file instead of fakes/ce_fakes.cc.
Contributor
Author
|
Non-inline findings from the ARGUS reviews of 80ce80f and 83b7faa are addressed in 221884b. CHANGELOG.md gets a 2.32.3 Resolved issues bullet for the CE ncclAlltoAllv hang, and the src/ce_coll.cc row of the MICROTEST_README.md fakes map now carries the "(targets that do not compile the real file)" qualifier. The microtest binary is also renamed to rccl-UnitTestsMicroCe in 395c787. |
Contributor
Author
|
@argus re-review |
…ift-AICOMRCCL-2474
Contributor
TheRock Submodule Bump ActivityNewest first
|
astyrrian1
pushed a commit
that referenced
this pull request
Oct 19, 2026
…ICOMRCCL-2474) (#12984) CE AlltoAllv deadlocks at 16+ ranks with uneven send sizes because each rank chose its intra-batch barrier count from its own nonzero peers. Fix: decide from the full size matrix and barrier per round of destination slots, so every rank issues the same count. Test results: - MI450 (gfx1250), 2 nodes, DPX, 16 ranks, rccl-tests alltoallv_perf default sparse matrix, 512M, -R 2: shipped RCCL 2.31.2 hangs 3/3, develop hangs 5/5, this fix passes 5/5 (~207 GB/s busbw). Uniform sizes pass 2/2 on develop. A 1M to 1G sweep hangs at exactly 512M on develop and passes every size with the fix. -G 1 also hangs on the shipped RCCL. - MI355X (gfx950), 1 node, 4 and 8 ranks, with NCCL_CUMEM_ENABLE=1 and a test-only sync frequency of 2 or 4 to reach the condition: develop hangs 15/15, the fix passes 15/15, controls pass, and bandwidth is unchanged. - Host microtest rccl-UnitTestsMicroCeColl: 40/40 pass with the fix, 20/40 fail on develop's ce_coll.cc; the full host suite passes. Hardware arms ran this fix before the capture-only parity barrier was added, a path CE AlltoAllv does not take since capture is not eligible. JIRA ID: AICOMRCCL-2474 (cherry picked from commit 014a242)
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.
CE AlltoAllv deadlocks at 16+ ranks with uneven send sizes because each rank chose its intra-batch barrier count from its own nonzero peers.
Fix: decide from the full size matrix and barrier per round of destination slots, so every rank issues the same count.
Test results:
Hardware arms ran this fix before the capture-only parity barrier was added, a path CE AlltoAllv does not take since capture is not eligible.
JIRA ID: AICOMRCCL-2474