feat(rccl-tests): GIN ReduceScatter host policy by dlamd1dai · Pull Request #13302 · ROCm/rocm-systems

dlamd1dai · 2026-10-12T20:17:53Z

Motivation

Split the host-side GIN Anvil-SDMA ReduceScatter policy out of #12747 so the CTA ladder, slice offset, and bandwidth math can be reviewed without the device kernel.

Technical Details

First of three stacked PRs that replace #12747. This one is based on develop.

gin_sdma_reducescatter_policy.h: per-rank slice sizing, size-adaptive CTA schedule, and bandwidth helpers.
usesSdmaTier: a grid below 16 CTAs is the SDMA-scatter tier. The default 32/48 ladder stays on LSA. Scratch sizing rejects a wrapping multiply.
rccl-UnitTestsGinSdmaReduceScatterPolicy: host unit test of those helpers, including the CTA pin and scratch overflow.
The device kernel and GPU test are the next PR. Pytest and GIN CI are the last.

NCCL_GIN_ANVIL_RS_CTAS is parsed here. The kernel PR is what consumes a pin below 16.

The low-CTA path came from fork draft dlamd1dai#17. It is opt-in. There is no bandwidth evidence yet that the pin beats the LSA ladder, and the default launch does not take it.

Issue Tracking

JIRA ID: AICOMRCCL-1803

Test Plan

rccl-UnitTestsGinSdmaReduceScatterPolicy passes (ctest -L unit), including usesSdmaTier, the pin-below-16 grid, and scratch overflow rejection
GIN CI execution of this binary is wired in the third PR

Test Result

Not re-run for the split. The pre-tier files match #12747 at f3606768a3. The CTA-pin commit is the additional change from fork PR 17.

Submission Checklist

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

Made with Cursor

Give ReduceScatter (-D 3) a host-testable CTA, slice, and bandwidth policy before the device kernel lands.

therock-pr-bot · 2026-10-12T20:18:54Z

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🚫 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?

A grid below 16 CTAs is the only way into the GIN-put path. The default 32/48 ladder stays on the LSA read-reduce. Scratch sizing rejects a wrapping multiply. AICOMRCCL-1803 Co-authored-by: Cursor <cursoragent@cursor.com>

amd-argus

ARGUS

Bottom line: Worth settling before approval: the header asserts the follow-up kernel's behaviour as present fact and names three functions absent from the tree, and reduceScatterDevReqs takes the raw -V count rather than the pool its own comment sizes the barriers from.

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

amd-argus · 2026-10-13T01:33:27Z

+// place that can be unit-tested on the host with no GPU (see
+// gin_sdma_reducescatter_policy_test.cpp). Companion to gin_sdma_allgather_policy.h.
+//
+// The device kernel calls the same helpers so the CTA schedule and requirement


Medium

This line says the device kernel calls these helpers so the CTA schedule is byte-for-byte what runs on the GPU, and lines 119, 174 and 226 name reduce_scatter.cu's getenv wrapper, ReduceScatterGetDevCommRequirements and ReduceScatterParseCtasEnv. None of those exist anywhere in the tree, and reduce_scatter.cu:70 still returns testNotImplemented for every deviceImpl != 0. Could we phrase these as the contract the follow-up kernel has to meet rather than as something already true, so the header reads honestly for anyone who opens it before PR 2 lands?

Resolved by 08040b012b.

amd-argus · 2026-10-13T01:33:27Z

+  bool supported;
+};
+GIN_SDMA_RS_HD inline DevReqs reduceScatterDevReqs(int deviceCtaCount) {
+  DevReqs r{deviceCtaCount, deviceCtaCount, deviceCtaCount, true, true};


Medium

reduceScatterDevReqs copies its argument straight into all three counts and names it deviceCtaCount, but line 172 says the pools are sized by reduceScatterPoolCtas(deviceCtaCount), so the natural call at -V 8 registers 8 slots while reduceScatterGridCtas hands back 48. The only test case, gin_sdma_reducescatter_policy_test.cpp:230, passes 32, which is a ladder constant, so it cannot tell the two readings apart. Nothing calls this yet so it cannot misbehave today, but naming the parameter for what it must be would stop the follow-up kernel reintroducing the exact overflow that gin_sdma_reducescatter_policy_test.cpp:204 exists to guard against.

Suggested change

DevReqs r{deviceCtaCount, deviceCtaCount, deviceCtaCount, true, true};

GIN_SDMA_RS_HD inline DevReqs reduceScatterDevReqs(int poolCtas) {

DevReqs r{poolCtas, poolCtas, poolCtas, true, true};

Resolved by 08040b012b.

amd-argus · 2026-10-13T01:33:27Z

+  return (c > poolCtas) ? poolCtas : c;
+}
+
+// Bytes of scratch-window the SDMA-scatter tier needs: N incoming per-source


Low

This comment describes reduceScatterSdmaScratchBytes at line 201, which multiplies by nRanks, but it sits above reduceScatterScratchBytes, which rounds exactly one value. The ambiguity is load-bearing rather than cosmetic: sdmaScratchFits at line 109 checks scratchBytes >= sliceBytes_ * nRanks with no rounding at all, so a reader cannot tell from the header whether the 128 B granularity is meant per slot or only for the whole window. Could we move it down and say which of the two it is, before the follow-up kernel picks a slot layout?

Resolved by 08040b012b.

amd-argus · 2026-10-13T01:33:27Z

+  return (size_t)v;
+}
+
+inline size_t parseReduceScatterCtasEnv(const char* name) {


Low

parseReduceScatterCtasEnv takes the variable name as an argument, so the literal NCCL_GIN_ANVIL_RS_CTAS exists in the whole checkout only inside comments, and a typo in the eventual caller would silently disable the knob with nothing to catch it. The sibling keeps the documented name and the parsed name together by hardcoding both of its names inside parseAllGatherCtasEnv at gin_sdma_allgather_policy.h:172. Could we add a named constant for the string here, which gets the same property without changing the signature?

Resolved by 08040b012b.

amd-argus · 2026-10-13T01:33:28Z

+  EXPECT_TRUE(sdmaScratchFits(1024, 8, 1024 * 8));
+  EXPECT_FALSE(sdmaScratchFits((size_t)-1, 8, (size_t)-1));
+  EXPECT_EQ(reduceScatterSdmaScratchBytes(8, kReduceScatterSdmaSlotMaxDefault),
+            reduceScatterScratchBytes(8ull * kReduceScatterSdmaSlotMaxDefault));


Low

reduceScatterSdmaScratchBytes is defined at gin_sdma_reducescatter_policy.h:204 as return reduceScatterScratchBytes((size_t)nRanks * slotMax);, which is exactly what the two sides of this EXPECT_EQ compute, so it holds no matter what the rounding policy turns out to be. Asserting the expected byte count directly, or covering the two arms the function actually has at gin_sdma_reducescatter_policy.h:202 and :203, neither of which any case reaches, would catch something.

Resolved by 08040b012b.

amd-argus · 2026-10-13T01:33:28Z

+// (16/eltSize elements). eltSize must be a power-of-two divisor of 16
+// (1,2,4,8,16), matching wordSize() of the rccl-tests element types. Mirrors
+// ReduceScatterGetCollByteCount. Returns 0 if inputs are degenerate.
+GIN_SDMA_RS_HD inline size_t sliceBaseCount(size_t count, size_t eltSize, int nranks) {


Low

sliceBaseCount is gin_sdma_allgather::chunkBaseCount character for character including its two inline comments, and sliceBytes, reduceScatterSliceOffset, pickSdmaThreshold, the MaxCtas/PoolCtas pair and bandwidthGBps are the same story, about 55 lines of bodies and closer to 95 once the paraphrased comment blocks are counted. The env parsers are the sharpest case: there are now four across the three headers and they disagree, since parseEnvU64 at gin_sdma_allgather_policy.h:161 and parseSize at gin_sdma_broadcast_policy.h:94 both accept "8foo" as 8 while this one rejects it. Not something to fix in this PR, but a shared gin_sdma_policy_common.h would let all three collectives inherit the strongest variant instead of whichever one got copied, and this looks like the moment to raise it as a follow-up.

amd-argus · 2026-10-13T01:33:28Z

+    target_link_libraries(rccl-UnitTestsGinSdmaReduceScatterPolicy PRIVATE ${GTEST_BOTH_LIBRARIES} Threads::Threads)
+    set_target_properties(rccl-UnitTestsGinSdmaReduceScatterPolicy PROPERTIES
+      RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/test")
+    add_test(NAME rccl-UnitTestsGinSdmaReduceScatterPolicy COMMAND rccl-UnitTestsGinSdmaReduceScatterPolicy)


Low

Line 23 of this file states that nothing in RCCL CI runs ctest, and the three sibling policy binaries are each enumerated in the test_runner configs, for instance rccl-UnitTestsGinSdmaAllGatherPolicy and rccl-UnitTestsGinSdmaBroadcastPolicy at tools/scripts/test_runner/configs/mi455_ainic_roce_func.json:2307 and :2323, while rccl-UnitTestsGinSdmaReduceScatterPolicy appears in none of the six. That matches the description's plan to wire GIN CI in the third stacked PR, so noting it rather than asking for the JSON change here: until then the Test Plan's ctest -L unit line is a local claim and a regression in these helpers would land green.

reduceScatterDevReqs took the raw -V count, so -V 8 registered 8 barrier slots while the ladder launches 48 CTAs. It now sizes from reduceScatterPoolCtas. That is idempotent, so callers that already pass the pool are unchanged. Name the NCCL_GIN_ANVIL_RS_CTAS knob once and default the parser to it. State that scratch is rounded per window, not per slot. Phrase the header as the contract the -D 3 kernel must meet. Replace the tautological scratch assertion with concrete sizes. AICOMRCCL-1803 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.}

feat(rccl-tests): add the GIN ReduceScatter host policy

2a7c1ef

Give ReduceScatter (-D 3) a host-testable CTA, slice, and bandwidth policy before the device kernel lands.

dlamd1dai requested review from a team October 12, 2026 20:17

This was referenced Oct 12, 2026

feat(rccl-tests): GIN Anvil-SDMA ReduceScatter (-D 3) #13303

Open

test(rccl): GIN CI and pytest for ReduceScatter (-D 3) #13304

Open

feat(rccl-tests): GIN Anvil-SDMA ReduceScatter (-D 3) #12747

Closed

github-actions Bot added project: rccl project: rccl-tests labels Oct 12, 2026

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

dlamd1dai self-assigned this Oct 12, 2026

feat(rccl-tests): select the ReduceScatter SDMA tier by CTA pin

d504085

A grid below 16 CTAs is the only way into the GIN-put path. The default 32/48 ladder stays on the LSA read-reduce. Scratch sizing rejects a wrapping multiply. AICOMRCCL-1803 Co-authored-by: Cursor <cursoragent@cursor.com>

dlamd1dai mentioned this pull request Oct 12, 2026

draft: feat(rccl-tests): low-CTA GIN SDMA ReduceScatter tier dlamd1dai/rocm-systems#17

Closed

5 tasks

amd-argus Bot reviewed Oct 13, 2026

View reviewed changes

amd-argus Bot reviewed Oct 20, 2026

View reviewed changes

	DevReqs r{deviceCtaCount, deviceCtaCount, deviceCtaCount, true, true};
	GIN_SDMA_RS_HD inline DevReqs reduceScatterDevReqs(int poolCtas) {
	DevReqs r{poolCtas, poolCtas, poolCtas, true, true};

Conversation

dlamd1dai commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Issue Tracking

Test Plan

Test Result

Submission Checklist

Uh oh!

therock-pr-bot Bot commented Oct 12, 2026

✅ All Policy Checks Passed

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot Oct 13, 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!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

dlamd1dai commented Oct 12, 2026 •

edited

Loading