fix(RCCL):Improve DDA LL reduce-scatter performance by increasing communication overlap. by JhaShweta1 · Pull Request #13503 · ROCm/rocm-systems

JhaShweta1 · 2026-10-14T20:57:08Z

Motivation

Improve DDA LL reduce-scatter performance by increasing communication overlap.

Technical Details

Overlap peer receives and stores.
Align shared scratch banks to prevent LL tier overwrites.
Add scratch-size eligibility tests.

Issue Tracking

JIRA ID: AICOMRCCL-2472

Test Plan

Run reduce_scatter_perf with validation enabled for sizes up to 1 MiB.
Run scratch-size eligibility test.

Test Result

Satisfactory performance
Passed test

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

therock-pr-bot · 2026-10-14T20:58:07Z

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	⚠️ Warning	Error: Source/code files changed without an accompanying unit test. Expected: add at least one test file named like `test_<name>.py` / `test_<name>.cpp` (or `<name>_test.`). Current:* code file(s) changed: `projects/rccl/src/algorithms/dda/all_reduce/all_reduce_dda_ll128_twoshot.h`, `projects/rccl/src/algorithms/dda/all_reduce/all_reduce_dda_ll_twoshot.h`, `projects/rccl/src/algorithms/dda/device/CollCommon.h`, `projects/rccl/src/algorithms/dda/reduce_scatter/reduce_scatter_dda_fabric_ll.h`, `projects/rccl/src/algorithms/dda/reduce_scatter/reduce_scatter_dda_fabric_ll128.h` (+3 more); no test file found
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

amd-argus · 2026-10-14T21:17:46Z

@cppccppccppc @sunguobao PTAL

amd-argus

ARGUS

Findings

5 more finding(s), not posted inline

projects/rccl/test/DdaFabricEligibilityTests.cpp:1548 (Low) ReduceScatterLL_BankOneMatchesSharedScratchMidpoint only re-checks the ddaLLRsBankOffsetPkts formula and would pass with the launcher fix reverted
projects/rccl/test/DdaFabricEligibilityTests.cpp:1546 (Low) EXPECT_LT(packedBank, bank) cannot fail at the derived 256/512 MiB scratch; the RS LL scratch boundary at .cu:127 is untested
projects/rccl/src/algorithms/dda/reduce_scatter/reduce_scatter_dda_fabric_ll.h:171 (Low) Phase-2 builtin branch reimplements ddaLLLoadLineB128 inline and recomputes peer/src in each loop
projects/rccl/src/algorithms/dda/reduce_scatter/reduce_scatter_dda_fabric_ll.h:58 (Low) kDdaLLRsPeerBatch adds a third copy of the peer-batch constant that all_reduce_dda_ll_twoshot.h:50 has a TODO to share
projects/rccl/src/algorithms/dda/reduce_scatter/dda_reduce_scatter_fabric_ll.cu:29 (Low) ddaLLRsScratchSize comment calls 2*ddaLLRsBankBytes the footprint, but it is now the minimum scratch for bank 1 at the midpoint

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

srirajpaul

is the performance data available ?

Gather each peer shard into registers before issuing the stores, matching the two-shot all-reduce, so the remote writes stay in flight together. Co-authored-by: Cursor <cursoragent@cursor.com>

…banks Issue each peer group's reads before testing flags, and place bank 1 at the shared scratch midpoint so reduce-scatter cannot overwrite another LL tier. Co-authored-by: Cursor <cursoragent@cursor.com>

Share the peer-batch width and drop repeated layout comments from review. Co-authored-by: Cursor <cursoragent@cursor.com>

amd-argus

ARGUS

Bottom line: Worth settling before approval: ReduceScatterLL_ScratchBanksFit only probes scratch = 2 * bankBytes, where the old and new bank-1 offsets coincide, so the bank move this PR makes is not pinned by any test.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

16- and 32-GPU sweeps show LL128 ahead of LL from 512 KiB per rank and ahead of ring through 1 MiB per rank. The old 512 KiB LL128 cap sat inside the 1 MiB LL window, so LL128 never ran. Co-authored-by: Cursor <cursoragent@cursor.com>

amd-argus

ARGUS

Bottom line: Nothing new worth raising; still open from earlier: DdaFabricEligibilityTests.cpp:1803. Worth clearing before approval.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

pvallem · 2026-10-20T00:20:40Z

+// Phase-2 reads in flight at once. Each peer keeps two data words and two
+// flags, so 4 stays in registers; 8 matches the send batch but costs twice
+// the temporary values.
+constexpr int kDdaLLRsRecvBatch = 4;


Did you try a receive batch of 8? At 8 ranks it costs about 14 VGPRs, with no spills and no occupancy limit. All 7 polls would then be in flight at once.

Tried a receive batch of 8 on a DPX node (8 gfx1250 ranks, one MPI process per GPU, confirmed DDA fabric LL). Across the LL window, the 8-rank float geomean bus bandwidth was +1.3% and bf16 +0.5%, both inside run-to-run noise. The 4-rank float sweep was −2.5%, since only 3 remote peers exist and the larger batch adds no extra polls. Leaving kDdaLLRsRecvBatch at 4.

pvallem · 2026-10-20T00:20:41Z

+        if (r < nRanks) {
+          const int peer = (selfRank + r) % nRanks;
+          const uint32_t* src = reinterpret_cast<const uint32_t*>(myBase + (size_t)peer * slot + pk);
+          while (f0[i] != flag || f1[i] != flag) {


This loop retries late peers one at a time. Could we re-poll all the stale lines in the batch together, so each retry round is a single round trip?

The first read of the batch is already issued together, and a line whose flags match is kept. The one-at-a-time loop only runs for a miss. Re-polling stale lines together would need the same open-coded global_load_b128 plus memory clobber as the first read, because ddaLLLoadLineB128 starts with a compiler barrier and would split the retries apart again. The batch-8 sweep already showed that extra in-flight polls do not move the LL window, so the retry is left one peer at a time.

pvallem · 2026-10-20T00:20:42Z

+        const int r = base + i;
+        if (r < nRanks) {
+          const int peer = (selfRank + r) % nRanks;
+          LLPacket16* dst = reinterpret_cast<LLPacket16*>(peerScratch[peer]) + bankOffsetPkts + (size_t)selfRank * slot;


The memory clobber in ddaLLStoreLineB128 forces peerScratch[peer] to be reloaded before every store. Could we compute the dst pointers once, outside the pk loop?

The reload is there: on gfx1250 the float 8-rank kernel does an s_load of peerScratch[peer] before each store. A cache computed once outside the packet loop was compiled back into those loads. Pinning the bases in scalar registers removed the reloads, with scratch still 0 and the float 8-rank kernel moving from 24 to 32 VGPRs. On the DDA LL window that was slower: 8-rank float geomean −1.7%, bf16 −3.0%, and 4-rank float −4.3% at every size. Leaving the per-store reload.

The 16 MiB LL total covered the 2 MiB LL128 window, so 4-GPU auto stayed on LL through 1 MiB per rank. LL128 is 1.5-1.9x faster from 512 KiB per rank. Co-authored-by: Cursor <cursoragent@cursor.com>

amd-argus

ARGUS

Bottom line: Nothing new worth raising; still open from earlier: DdaFabricEligibilityTests.cpp:1803. Worth clearing before approval.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

At two banks the packed offset and bankSize agree, so the existing eligibility test cannot tell them apart. Co-authored-by: Cursor <cursoragent@cursor.com>

amd-argus

ARGUS

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

…_ll_rs Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # projects/rccl/src/algorithms/dda/reduce_scatter/dda_reduce_scatter_fabric_ll.cu # projects/rccl/src/algorithms/dda/reduce_scatter/reduce_scatter_dda_fabric_ll.h # projects/rccl/test/DdaFabricEligibilityTests.cpp

amd-argus

ARGUS

Bottom line: LGTM once dda-reduce-scatter-fabric-ll-test.cc expects the new bankSize launch argument (10 args, bankSize at index 8, params at 9).

_{Comment @argus review to re-run, or @argus re-review after pushing.}

The launcher passes bankSize before DdaKernelParams, so the microtest still checking nine arguments fails. Co-authored-by: Cursor <cursoragent@cursor.com>

amd-argus

ARGUS

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

JhaShweta1 requested review from a team October 14, 2026 20:57

github-actions Bot added the project: rccl label Oct 14, 2026

JhaShweta1 requested a review from nusislam October 14, 2026 21:03

systems-assistant Bot added the organization: ROCm label Oct 14, 2026

amd-argus Bot reviewed Oct 14, 2026

View reviewed changes

Comment thread projects/rccl/test/DdaFabricEligibilityTests.cpp Outdated

nusislam mentioned this pull request Oct 15, 2026

feat(rccl): Add LL128 support for DDA RS #12928

Merged

1 task

nusislam reviewed Oct 16, 2026

View reviewed changes

Comment thread projects/rccl/src/algorithms/dda/reduce_scatter/reduce_scatter_dda_fabric_ll.h Outdated

Comment thread projects/rccl/test/DdaFabricEligibilityTests.cpp Outdated

nusislam requested a review from srirajpaul October 16, 2026 18:32

srirajpaul reviewed Oct 16, 2026

View reviewed changes

Comment thread projects/rccl/src/algorithms/dda/reduce_scatter/reduce_scatter_dda_fabric_ll.h

Shweta Jha and others added 2 commits October 19, 2026 16:34

perf(rccl): overlap DDA LL reduce-scatter peer stores

f236c84

Gather each peer shard into registers before issuing the stores, matching the two-shot all-reduce, so the remote writes stay in flight together. Co-authored-by: Cursor <cursoragent@cursor.com>

perf(rccl): overlap DDA LL reduce-scatter receives and align scratch …

3603a0a

…banks Issue each peer group's reads before testing flags, and place bank 1 at the shared scratch midpoint so reduce-scatter cannot overwrite another LL tier. Co-authored-by: Cursor <cursoragent@cursor.com>

JhaShweta1 force-pushed the users/shwetjha/dda_ll_rs branch from 6a94138 to 3603a0a Compare October 19, 2026 16:55

fix(rccl): check DDA LL reduce-scatter scratch in bank units

e625952

Share the peer-batch width and drop repeated layout comments from review. Co-authored-by: Cursor <cursoragent@cursor.com>

amd-argus Bot reviewed Oct 19, 2026

View reviewed changes

Comment thread projects/rccl/test/DdaFabricEligibilityTests.cpp

amd-argus Bot reviewed Oct 19, 2026

View reviewed changes

pvallem reviewed Oct 20, 2026

View reviewed changes

Comment thread projects/rccl/test/DdaFabricEligibilityTests.cpp

pvallem reviewed Oct 20, 2026

View reviewed changes

Comment thread projects/rccl/src/algorithms/dda/reduce_scatter/reduce_scatter_dda_fabric_ll.h Outdated

pvallem reviewed Oct 20, 2026

View reviewed changes

perf(rccl): open DPX reduce-scatter LL128 below 8 ranks

3d9b061

The 16 MiB LL total covered the 2 MiB LL128 window, so 4-GPU auto stayed on LL through 1 MiB per rank. LL128 is 1.5-1.9x faster from 512 KiB per rank. Co-authored-by: Cursor <cursoragent@cursor.com>

amd-argus Bot reviewed Oct 20, 2026

View reviewed changes

test(rccl): check the LL reduce-scatter bank offset at a larger scratch

5103312

At two banks the packed offset and bankSize agree, so the existing eligibility test cannot tell them apart. Co-authored-by: Cursor <cursoragent@cursor.com>

amd-argus Bot reviewed Oct 21, 2026

View reviewed changes

Comment thread projects/rccl/src/algorithms/dda/reduce_scatter/dda_reduce_scatter_fabric_ll.cu

test(rccl): expect bankSize in the LL reduce-scatter launch

b3fc3d8

The launcher passes bankSize before DdaKernelParams, so the microtest still checking nine arguments fails. Co-authored-by: Cursor <cursoragent@cursor.com>

amd-argus Bot reviewed Oct 21, 2026

View reviewed changes

Conversation

JhaShweta1 commented Oct 14, 2026

Motivation

Technical Details

Issue Tracking

Test Plan

Test Result

Submission Checklist

Uh oh!

therock-pr-bot Bot commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ All Policy Checks Passed

Uh oh!

amd-argus Bot commented Oct 14, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

srirajpaul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pvallem Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

JhaShweta1 Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

pvallem Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

JhaShweta1 Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

pvallem Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

JhaShweta1 Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

therock-pr-bot Bot commented Oct 14, 2026 •

edited

Loading