Repository navigation
perf(rccl): batch peer polling and prefetch sends in the DDA LL one-shot AllReduce - #14880
Merged
pvallem merged 2 commits intoNov 4, 2026
Merged
Conversation
…hot AllReduce The one-shot read a cold send buffer after its epoch barrier and waited on each peer line in turn, so the round trips serialized. Prefetch the send packets into L2 before the barrier on gfx1250, and poll each batch of peer lines under one load wait.
❌ PR Check — Action Required
📖 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 |
pvallem
approved these changes
Nov 3, 2026
pvallem
approved these changes
Nov 3, 2026
Contributor
|
@cppccppccppc @sunguobao PTAL |
![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/algorithms/dda/device/CollCommon.h:482(Low)line[__builtin_ctz(mask)]indexesline[]with a runtime value, which CollCommon.h:322-323 says puts the array in scratch; pickfirstwith an unrolled select instead.projects/rccl/src/algorithms/dda/device/CollCommon.h:475(Low) Follow-up: ddaLLWaitLineFlagsBatched and ddaLLPrefetchSend duplicate the inline poll and prefetch in reduce_scatter_dda_fabric_ll.h (:85-93, :138-193), which already use a different batch size (4 vs 8); the ReduceScatter path could call the shared helpers.projects/rccl/test/device/DdaAllReduceFabricLLTests.cpp:127(Low) bitsOf, phase1Done, recv, expectEpochAdvanced and the phase-1 wait loop copy the ReduceScatter test's helpers; they could move into DdaLLHostPeers.hpp.projects/rccl/test/device/DdaReduceScatterFabricLLTests.cpp:10(Nit)#include <atomic>is unused now that the release fences live in DdaLLHostPeers.hpp; drop it.#include <atomic>has no remaining user now that the release fences live in DdaLLHostPeers.hpp, so it can be dropped.projects/rccl/test/CMakeLists.txt:314(Nit) This comment now heads two files; change 'it runs' to 'they run'.
Bottom line: Worth settling before approval: DdaAllReduceFabricLL.* is missing from both test_categories yaml files, and no test runs more than 8 ranks, so the multi-batch fold and the prefetch bound go unchecked.
Comment @argus review to re-run, or @argus re-review after pushing.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
…catter poll barriers The DDA LL one-shot AllReduce inlines its batched poll and fold, hard-codes the prefetch group as 16 lanes, pins each polled word with inline asm, indexes the line array with a runtime value, and its device tests are not in the test categories. Fix: move the poll and rank-order fold into ddaLLFoldRanksInOrder next to the AllReduce kernel, derive the prefetch group from the 128 B line and 8 B packet sizes, load each sweep into a v4u array between the same asm memory barriers the ReduceScatter LL kernel uses, pick the first polled line with an unrolled select, and list DdaAllReduceFabricLL.* in test_categories.yaml and test_categories_fixtures.yaml.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
alex-breslow-amd
approved these changes
Nov 3, 2026
alex-breslow-amd
left a comment
alex-breslow-amd
left a comment
Contributor
There was a problem hiding this comment.
This looks improved.
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 DDA LL one-shot AllReduce reads a cold send buffer after its epoch barrier, and waits on each peer line in turn, so the round trips serialize.
Technical Details
ddaLLPrefetchSend).ddaLLWaitLineFlagsBatched) instead of one line at a time. With 5 or more lines in a batch, a waiting lane spins on one stale line between sweeps.recvbuffwrite and still advances the epoch.DdaLLHostPeers.hpp).Issue Tracking
N/A
Test Plan
DdaAllReduceFabricLL.*device tests, in rccl-UnitTests (b128 loads) and rccl-UnitTestsFixtures (dword loads): 4 and 8 ranks compile-time, 2 and 6 ranks runtime, late peer, torn line, abort while polling.DdaReduceScatterFabricLL.*tests.Test Result
Submission Checklist