[dlamd1dai]

Replaces #11569. Identical commits and content; head is cf8cf9720f16391c76a19534b2af34eaea0c67cd on both.

#11569 was opened from a fork, where the required RCCL PR gate check can never pass. Fork pull requests receive no secrets, so the RCCL coco PR workflow skips its Setup job, the coco smoke never runs, and the gate fails closed within seconds. Clearing it there needs a maintainer to dispatch the workflow by hand and override the check, on every push. This branch lives in ROCm/rocm-systems, so the gate dispatches coco to the Ruby and OCI clusters on its own.

Review history stays on #11569: 27 threads, all resolved. No approval was lost; it had no approval or changes-requested decision.

Motivation

Anvil SDMA currently allocates one GPU context per connection and ignores nContexts. When ncclGinAllContexts barriers run on a shared connection, every logical context writes and waits on the same signal cells, so a later barrier can complete on a stale count from an earlier one.

Technical Details

Give each logical context its own ncclGinAnvilSdmaGPUContext and LSA signal stripe while keeping SDMA queues, dirty bits, and counters shared at the connection. Device APIs now index ctx.handle by ctx.contextId. Resource-window bind walks nContexts slots instead of one slot per plugin context.

Signal peer addresses are indexed in GIN-team space (ctx->rank), matching resolveRemotePeerVa; regMrSym keeps devr->lsaSelf because that table is LSA-indexed. The signal span is registered in the IPC table once per bind rather than once per context, and the comm-wide collectives (the local-base allgather and the LSA conn-check) run at span scope so a comm with N logical contexts still costs one bootstrap round trip, not N.

Two fixes to the surrounding code are folded in, previously proposed standalone as #12450 and #12451 and closed in favour of this PR once the restructuring above made them inapplicable as separate patches:

• The span base probe is sized with comm->nRanks and publishes the local VA at comm->rank. bootstrapAllGather writes one slot per world rank, so a GIN-team-sized buffer was overrun and the VA landed in the wrong slot under NCCL_GIN_CONNECTION_RAIL.
• The bind path snapshots g_pendingByComm into a vector under pluginMutex and walks the copy. Its three reads (span geometry, slot binding, and fail: rollback) were unlocked while ginAnvilPendingAdd/Remove/Clear all hold that non-recursive mutex; find() also avoids default-inserting an empty list for a comm with nothing pending.

JIRA ID: AICOMRCCL-2339
AICOMRCCL-3072

Test Plan

• Host plugin unit tests, including BindSignals_LogicalContextsUseDistinctSignalStripes and the ConnCheck_* suite
• Device template tests, including SignalApis_SelectLogicalContext and PutValue_SelectLogicalContext
• MPI regression Barrier_AllContextsConsecutiveSignalsDoNotAlias_SingleNode (Anvil, 1 node / 2 ranks, NCCL_GIN_TYPE=7, delayed second barrier)

Test Result

Anvil SDMA is NCCL_GIN_TYPE=7 (NCCL_NET_DEVICE_GIN_ANVIL_SDMA). Earlier runs in this section were recorded when the enum was 6.

Pre-rebase (commit 970a014315):

• rccl-UnitTestsGinAnvilPlugin: 20/20 passed on MI355
• GinAnvilSdmaTemplateTest.*: 13/13 passed on MI355 (before PutValue_SelectLogicalContext was added)
• GinMPIDeviceTests.Barrier_AllContextsConsecutiveSignalsDoNotAlias_SingleNode on MI355: PASSED (7779 ms) on smci355-ccs-aus-m03-17, 1 node / 2 ranks, NCCL_GIN_ENABLE=1, NCCL_CUMEM_ENABLE=1. Ran rccl-UnitTestsMPI inside rccl-gin-pr11556-rs:latest with --ipc=host. Host mpirun on the same node segfaulted in HIP IPC during communicator bring-up (ncclP2pImportShareableBuffer / ncclDevrCommCreateInternal) while other GPU docker jobs held /dev/kfd and /dev/dri; that path never reached the kernel.
• Same MPI filter on MI350: PASSED (8834 ms) on cv350-rck-g03-e23-08, 1 node / 2 ranks, same GIN env. Host mpirun -np 2; GPUs were idle. Used the gfx950 rccl-UnitTestsMPI/librccl.so.1 from the MI355 PR build (native CentOS 9 link of librccl.so failed on librocshmem_gin_host.a bitcode encoding; Docker was unavailable on that node).

Current head cf8cf9720f, rebased onto develop at 482e541856, retested from a clean worktree on MI355X (smci355-ccs-aus-m03-17), 1 node / 2 ranks, NCCL_GIN_TYPE=7, NCCL_GIN_ENABLE=1, NCCL_CUMEM_ENABLE=1, NCCL_GIN_NCONTEXTS=4:

• rccl-UnitTestsGinAnvilPlugin: 37/37 passed (304 ms), including the full ConnCheck_* suite and the two span-scope cases added for the ARGUS finding. Both were confirmed to fail against 8eafd3d752 (3 allgathers at nContexts=3) and pass here (1).
• Whole GinMPIDeviceTests.* suite on the Anvil backend: 44 passed, 20 skipped, 0 failed (40.1 s). Covers Barrier_AllContextsConsecutiveSignalsDoNotAlias_SingleNode (680 ms), all seven BarrierFence_*_SingleNode cases including AllContextsPut/AllContextsGet, MultiContext_AllFourRoute/NonPowerOf2/Exclusive, and Put_BasicAndOffsets_MultiContext/PutValue_Inline_MultiContext/WaitCounterAndSignal_MultiContext.
• The 20 skips are topology or opt-in gated, not regressions: cross-node and 4-rank cases, RailConnection_Create, the VASignal_* cases (VA signals are unsupported on SDMA by design), and the pool tests that require NCCL_GIN_SIGNAL_POOL_SIZE>=0x40000000.

An earlier attempt at this run skipped all 64 cases because NCCL_CUMEM_ENABLE was unset, which trips the Symmetric memory required gate; the numbers above are from the corrected invocation.

Earlier post-rebase run on f32754f6c1, same node and env, before #12450/#12451 were folded in: plugin 37/37, GinMPIDeviceTests.* 39 passed, 23 skipped, 0 failed (34.5 s).

Environment note: the host ROCm on that node is 7.0.2.1 while its KFD driver is newer, and hipIpcOpenMemHandle segfaults during plain P2P transport setup (ncclP2pImportShareableBuffer) before any GIN code runs. The runs above use the node's ROCm 7.13 runtime in a container with --ipc=host, where the same binaries pass. Not related to this change; the same workaround was needed for the pre-rebase runs.

Submission Checklist

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

Made with Cursor

[therock-pr-bot]

✅ 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?

[amd-argus]

ARGUS

Bottom line: Looks approval ready. The per-context stripe layout, span refcount balance and slot accounting all check out against the caller in dev_runtime.cc; everything flagged inline is Low or Nit.

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

[dlamd1dai]

@argus re-review

[amd-argus]

ARGUS

Findings

2 finding(s) that could not be posted as inline comments

• projects/rccl/test/transport/GinDeviceMPITests.cpp:2329 (Low) The AllContexts aliasing regression test goes vacuous whenever a rank has more than one GIN connection Barrier_AllContextsConsecutiveSignalsDoNotAlias_SingleNode gates only on ginContextCount >= 2, but the aliasing it reproduces needs two logical contexts on one connection: ncclGinInitCommon derives contextId = contextIndex / ginConnectionCount (gin__funcs.h:122-125), so at ginCommCount == 2 both contexts get contextId 0 on different handles and every assertion here passes with anvilGpuCtx reverted. MultiContext_NonPowerOf2 in this same config list already runs with NCCL_GIN_NCONNECTIONS=3. Could the guard pin contexts-per-connection too?
• projects/rccl/src/dev_runtime.cc:2218 (Low) The new fail-path teardown frees the GIN contexts but leaves ginContextCount and ginHandles[] populated The new fail-path teardown frees the GIN contexts but leaves outDevComm->ginContextCount and ginHandles[] populated, so ncclDevCommDestroy on the failed struct re-enters ncclGinDevCommFree (dev_runtime.cc:2699) and matches on a devHandles[0]->handle this call already freed (gin_host.cc:471). The GIN setup path clears exactly these fields on its own error path (gin_host.cc:407-419). Could the fail path mirror that?

Bottom line: Looks approval ready. All six findings from the last round are fixed; the four inline notes are Low, mostly about the new regression test being able to go vacuous.

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

[amd-argus]

ARGUS

Findings

2 finding(s) that could not be posted as inline comments

• projects/rccl/src/dev_runtime.cc:2081 (Medium) GIN signal shadows are sized with the unrounded context count but indexed with the rounded one dev_runtime.cc:2081 sizes the GIN signal-shadow region with the unrounded nGinContexts, while the device indexes it with comm.ginContextCount, which gin_host.cc:268 sets to ROUNDUP(reqs->ginContextCount, ginCommCount). ''' reserved dev_runtime.cc:2081 nGinContexts = 4 slots indexed gin__funcs.h:129 ROUNDUP(4, ginCommCount=3) = 6 slots overflow lands at ginAnvilNetSignalsOffset (dev_runtime.cc:2082) ''' The comment at dev_runtime.cc:2084 says the rounding exists so shadows stay in range, but it is applied to the Anvil span at 2091 instead. This predates the PR and line 2081 is untouched here, so it looks like a separate change.
• projects/rccl/CHANGELOG.md:47 (Low) No CHANGELOG entry for a user-visible GIN device API fix projects/rccl/CHANGELOG.md has no entry for this fix. The unreleased 2.32.3 section's ### Resolved issues list ends at line 47 and already carries a comparable GIN bugfix at line 46, and .github/PULL_REQUEST_TEMPLATE.md:20 makes the CHANGELOG an approval-checklist item when changes impact library users. Consecutive ncclGinAllContexts barriers returning early on a stale count is user-visible, so one bullet after line 47 would cover it.

Bottom line: Looks approval ready. All four findings from the last round are fixed; the new ones are Low apart from one Medium gap in test coverage.

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

[dlamd1dai]

@argus re-review

[dlamd1dai]

Thanks for the thorough re-review. Replies to the two findings that could not be posted inline:

• Signal-shadow sizing (dev_runtime.cc:2081, Medium): Agreed, and thanks for spotting it. Since it predates this PR, the fix is in a separate PR, fix(devr): size GIN signal shadows for the rounded context count #13228. It sizes the shadow region and the Anvil span for the largest rounded context count over all active backends, because rounding to the widest backend is not enough (ROUNDUP(4, 4) < ROUNDUP(4, 3)).
• CHANGELOG (Low): Added a Resolved issues bullet in 3bc925d.

[amd-argus]

ARGUS

Findings

2 finding(s) that could not be posted as inline comments

• projects/rccl/test/transport/GinDeviceMPITests.cpp:2317 (Medium) The new MPI regression test omits SKIP_IF_GIN_UNSUPPORTED(), so a RAIL-only comm fails it instead of skipping Barrier_AllContextsConsecutiveSignalsDoNotAlias_SingleNode is missing SKIP_IF_GIN_UNSUPPORTED() after createTestCommunicator(). The four gates above it read only env vars and MPI topology, never globalGinSupport, so on a RAIL-only or GIN-less comm the defaultGinReqs() FULL request makes ncclDevCommCreate fail the test rather than skip it. Barrier_TwoRanks guards the same sequence at line 2204.
• projects/rccl/docs/userguide/source/api/device_gin.rst:272 (Medium) The user guide still documents AICOMRCCL-2339 as an open follow-up, contradicting this PR's own CHANGELOG entry The ncclGinAllContexts docs at docs/userguide/source/api/device_gin.rst:272 still say Anvil SDMA contexts share one signal array and call AICOMRCCL-2339 an open plugin follow-up, while this PR's CHANGELOG.md entry at line 48 says each logical context now has its own signals. Shipping both texts in one release leaves the user guide describing the bug this PR fixes. That file is outside the diff, so flagging rather than suggesting.

This PR changes 1,009 lines across 17 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.

Bottom line: Worth settling before approval: the new MPI test hard-fails instead of skipping on a RAIL-only comm, and device_gin.rst still documents this bug as unfixed.

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

[dlamd1dai]

Thanks for the re-review. Replies to the two findings that could not be posted inline:

• Missing SKIP_IF_GIN_UNSUPPORTED() in the MPI alias test (Medium): Added right after createTestCommunicator() in e14907e, matching Barrier_TwoRanks.
• device_gin.rst still describing AICOMRCCL-2339 as open (Medium): Removed the Anvil SDMA shared-signal note in e14907e, so the user guide now matches the CHANGELOG.

On the size note: thanks for the suggestion. I'd like to keep this as one PR, since the plugin change and its tests only make sense together and most of the line count is tests.

MI355 retest on e14907e: rccl-UnitTestsGinAnvilPlugin 43/43, and the MPI alias test passes.

[dlamd1dai]

@argus re-review

[amd-argus]

ARGUS

This PR changes 1,079 lines across 18 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.

Bottom line: LGTM.

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

[amd-argus]

ARGUS

This PR changes 1,079 lines across 18 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.

Bottom line: LGTM.

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

[dlamd1dai]

Pushed 908015bbba: host-side cleanup coverage for Anvil logical contexts. It mirrors the multi-context allocation, striping and cleanup coverage requested on the GDA counterpart (#12746). Striping was already covered here by BindSignals_LogicalContextsUseDistinctSignalStripes; cleanup was not. Test-only change; gin_plugin_anvil_sdma.cc is untouched.

• DestroyContext_ReleasesEveryLogicalContextAllocation: with 3 logical contexts bound, destroyContext frees the GPU context array, the counter span, and each context's signal_remote_addrs. The connection-owned SDMA queue handles and dirty word stay alive.
• CreateContext_CounterAllocFailureReleasesContextArray: a failed counter-span allocation makes createContext return ncclSystemError and free the GPU context array that was already allocated.
• BindSignals_SlotOutOfRangeInsideLogicalContexts now also checks that destroy frees the peer tables contexts 0 and 1 bound before slot 2 overflowed.

HIP keeps freed blocks mapped, so pointer queries cannot detect a free. The plugin stubs therefore interpose hipMalloc / hipExtMallocWithFlags / hipFree (forwarding via dlsym(RTLD_NEXT)), keep a live-allocation ledger, and can fail the next hipExtMallocWithFlags once. The counts exclude the process-lifetime IPC device table, which is allocated on the first span registration.

Verified on MI355X (gfx950): rccl-UnitTestsGinAnvilPlugin 45/45, also under --gtest_shuffle with three seeds. Each new assertion fails under a matching mutation: skipping the per-context peer-table free, freeing only context 0's table, skipping the counter or GPU-context-array free, leaking on the createContext error path, or freeing the connection's dirty word on destroy.

[amd-argus]

ARGUS

This PR changes 1,204 lines across 18 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.

Bottom line: LGTM.

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

[amd-argus]

ARGUS

This PR changes 1,204 lines across 18 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.

Bottom line: LGTM.

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

[amd-argus]

ARGUS

This PR changes 1,200 lines across 18 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.

Bottom line: LGTM.

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

[alex-breslow-amd]

Already has gone through reviews

[alex-breslow-amd]

Just noticed this failure in extra:

I'm curious if this is pre-existing or not.

[dlamd1dai]

@alex-breslow-amd Thanks for the approval, and for flagging this. It's pre-existing on develop, not caused by this PR.

The only failing case in MathCI precheckin(rccl), on both gfx942 and gfx1100, is NcclDeviceRequirementsMicrotest.GinInboxA2ACreateRequirement_KnownBug_SingleRankTeamDividesByZero ("failed to die"). It came in with #13312, merged into develop on Oct 20, and reached this branch through the rebase. This PR doesn't touch src/nccl_device/gin_scratch.cc, bitops.h, or the test.

I built rccl-UnitTestsMicroNcclDevice on MI355 from current develop (02d6cbc711) and from this PR's head (00ba764584), and the test fails the same way on both.

The likely cause is that the test #includes gin_scratch.cc, so ncclGinInboxA2ACreateRequirement can be inlined. The divide-by-zero result (nPeers_rcp32) is never read afterward, and division by zero is undefined behavior, so an optimized build is free to drop the div that should raise SIGFPE. I'll let the #13312 author know.

[aduforest]

Might be worth merging #13228 first or alongside, since each logical context now takes its own signal slot

[dlamd1dai]

Rebased onto develop (0587950a1c) and pushed c14f75b2cb. There were no conflicts. git range-diff shows all 14 commits are patch-identical to 76a0c25d3d, and the PR diff is unchanged (18 files, +1041/−160).

The rebase picks up #13927, which drops NcclDeviceRequirementsMicrotest.GinInboxA2ACreateRequirement_KnownBug_SingleRankTeamDividesByZero. That death test was the only failure in MathCI precheckin(rccl) #14, on gfx942, gfx1100, gfx950 and gfx908 (6404 passed, 1 failed). It is pre-existing on develop, as noted above.

@ROCm/rccl-ci, for the record on the previous head: RCCL PR gate failed only because OCI / oci-rccl-pr-smoke never launched coco. On oci-linux-slurm-scale-runner-2 (job), Install coco failed in python3 -m venv with:

The virtual environment was not created successfully because ensurepip is not
available.  On Debian/Ubuntu systems, you need to install the python3-venv
package using the following command.

    apt install python3.10-venv

Ruby smoke passed on the same run. Later OCI jobs on the same runner (from 15:30 UTC Oct 22) got past Install coco, so this may already be fixed. The push started a new RCCL coco PR run. If OCI hits the ensurepip error again, could you install python3.10-venv on that host and rerun the gate?

[systems-assistant]

TheRock Submodule Bump Activity

Newest first

• 2026-10-26 — Included in TheRock via ROCm/TheRock#9480.