refactor(rccl/gin): Simplify build for gin-anvil and remove sdma-induced rocshmem link dependencies by abouteiller · Pull Request #11757 · ROCm/rocm-systems

abouteiller · 2026-09-17T12:39:27Z

Motivation

Simplify consumer code linkage by not requiring linking rocshmem from rccl clients. This remove the SDMA induced link requirements (GDA requirements remain)

Technical Details

Files that are ingested from rocshmem sources are installed in the rccl install, or compiled directly inside rccl as needed. No rocshmem linkage required.

Issue Tracking

JIRA ID : AICOMRCCL-1467

Test Plan

dev build and run
GIN CI

Test Result

Can build rccl, rccl-tests on rocm 10.1 nightly
Can run rccl-tests alltoall sdma

Submission Checklist

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

to rocshmem (link dependencies induced by GDA remains TBD)

therock-pr-bot · 2026-09-17T12:40:15Z

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

therock-pr-bot · 2026-09-17T12:40:17Z

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

amd-argus

ARGUS

Bottom line: Worth settling before approval: ROCSHMEM_SOURCE_DIR is empty at src/CMakeLists.txt:876 and never reaches the top-level scope at CMakeLists.txt:833, so anvil.cpp is not compiled in and the SDMA headers are not installed.

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

Build system fixes: - [1] Move target_sources(anvil.cpp) after add_rocshmem_targets() so ROCSHMEM_SOURCE_DIR is set for raw cmake builds without install.sh - [2] Use CACHE INTERNAL for ROCSHMEM_SOURCE_DIR in ROCSHMEM.cmake so the top-level install rules can see it (no PARENT_SCOPE chain needed) - [3] Fix anvil.cpp include path: ${CMAKE_CURRENT_SOURCE_DIR}/gin (not src/gin) so log.hpp shim shadows rocshmem's log.hpp correctly - [7] Add missing #include <cstdint> in gda_enums.hpp for uint32_t Test fixes: - [4] Compile gin_anvil_sdma_factory.cc + anvil.cpp directly into the factory test (self-contained, no librccl.so link, no visibility attr) - [5] Add EnvVars.cpp and #include <new> for placement new - [9] Fix null-pointer UB in Create_NullOutParams: call factory API directly instead of dereferencing nullptr through tryCreate helper - Rename Create_MultiRankMock -> Create_MultiChannel (4-ch loopback); cross-device multi-rank needs RCCL's full init path - Replace memset on ncclComm with placement new (non-trivially copyable) - Fix hipError_t nodiscard warnings in plugin test stubs - Use hipified debug.h (not src/include) to avoid cudaError_t in HIP CI fixes: - [8] Fix factory test path in gin.sbatch (${rccl_build}/test/ not build/test/) and add rocm_sysdeps to LD_LIBRARY_PATH Cleanup: - Remove stale ibverbs find_library from Findrocshmem_static.cmake - install.sh: skip -DROCSHMEM_SOURCE_DIR in monorepo to exercise cmake auto-detect path Bracketed numbers [N] reference amd-argus bot review comments. Co-Authored-By: Claude <noreply@anthropic.com>

amd-argus

ARGUS

Bottom line: Worth settling before approval: rccl-UnitTestsGinAnvilSdmaFactory has no rocSHMEM build include dir, so gin_anvil_sdma_factory.cc pulls rocSHMEM's real log.hpp and its generated rocshmem_config.h, and the target does not compile.

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

…11757 Build fixes: - [10] Remove vestigial #include "log.hpp" from anvil_device.hpp — no LOGD_* macros remain after earlier cleanup, so the transitive include chain (factory.cc → anvil.hpp → anvil_device.hpp → log.hpp) no longer pulls in rocshmem's log.hpp during librccl.so compilation - [11] Add missing #include <cstdlib> in src/gin/log.hpp installed-context fallback — abort() requires it, <cstdio> alone is not sufficient Test fixes: - [8b] Add add_dependencies(rccl-UnitTestsGinAnvilSdmaFactory hipify_all) so hipified debug.h exists before the test compiles - [8c] Fix stale comment: factory test is self-contained (compiles sources directly), does NOT link librccl.so Dismissed: - [9] gin_anvil_sdma_get_num_channels returns the clamped value — test intentionally verifies clamping, not actual HW channel count Bracketed numbers [N] reference amd-argus bot review comments (round 2). Co-Authored-By: Claude <noreply@anthropic.com>

abouteiller · 2026-09-21T21:21:58Z

@argus re-review

amd-argus

ARGUS

Findings

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

projects/rccl/src/gin/log.hpp:17 (Low) The debug.h arm calls abort() without including <cstdlib> abort() on lines 19 and 20 is declared in <cstdlib>, but this arm includes only debug.h, whose own includes are nccl.h, nccl_common.h, <stdio.h>, compiler.h and <thread>. None of those is required to declare it; the #else arm gets this right at line 31. It compiles today only because anvil.cpp includes anvil.hpp, and its <iostream>, first.

Bottom line: Worth settling before approval: gin.sbatch:239 expands rccl_build outside the subshell that sets it, so set -u kills the job before step 7 and the existing GIN suite stops running too.

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

…11757 log.hpp fixes: - [r4037446399] LOG_ERROR_EXIT now calls exit(EXIT_FAILURE) matching rocshmem's semantics; LOG_ERROR_ABORT remains abort() - [r4037446287/17] Add #include <cstdlib> in debug.h arm for abort()/exit() - [r4037446410] Remove log.hpp from install rule — anvil_device.hpp no longer includes it, so the __has_include("debug.h") hazard is eliminated for installed consumers Factory header: - [r4043798119/r4037446331] Restore doxygen from the deleted rocshmem header: allgather contract, num_channels clamping, output handle indexing Test fixes: - [r4037446382] SetUp: guard hipGetDeviceCount against hipErrorNoDevice so validation tests still run on no-GPU hosts - [r4037446391] Mock allgather: pass nRanks via context pointer (the factory passes sizeof(int) as bytes_per_rank, not the buffer total — nRanks cannot be derived from it); add mockCtx() helper, tryCreate() auto-wires the context from its nRanks argument - [r4037446356] Remove orphaned gin_anvil_sdma_factory_test.cpp from rocshmem (moved to rccl/test/gin/GinAnvilSdmaFactory_test.cpp) CI fixes: - [r4037446372] Remove --tests_build from install.sh invocation — the comment explains the hipify race; build factory test via the existing second-step cmake reconfigure (BUILD_TESTS=ON, GIN_ANVIL_UNIT_TESTS=ON) - [r4037446361/r4037446375] Source env files (rocm/ompi/rocshmem/rccl) once before test steps so ROCM_PATH etc. are available outside the build subshell Dismissed: - [r4037446406] LOG_INFO asymmetry between arms is intentional - [r4037446420] LOGD stubs retained for future GDA device code use Bracketed IDs reference GitHub discussion comment IDs. Co-Authored-By: Claude <noreply@anthropic.com>

abouteiller · 2026-09-23T15:23:24Z

@argus re-review

…rccl-gin/sdma-build-simplification

amd-argus

ARGUS

Bottom line: Worth settling before approval: gin.sbatch:244 expands rccl_build, which is only set inside the build subshell, so under set -u the job dies at step 6 and the GIN tests never run.

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

Co-authored-by: amd-argus[bot] <321830313+amd-argus[bot]@users.noreply.github.com>

when linking sdma

alex-breslow-amd

Accept with nits

amd-argus

ARGUS

Bottom line: Worth settling before approval: gin.sbatch step 6 swallows the factory test's exit status, so a failing run leaves no tests-failed sentinel and the check stays green.

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

…rccl-gin/sdma-build-simplification Conflicts resolved: - src/CMakeLists.txt: keep both Omri's QP header copy list and our anvil.cpp comment; take Omri's rocshmem header copy block wholesale - gin_rocshmem_gda_factory.cc: keep gda_enums.hpp include (needed for GDAProvider and gda::vendor_id), remove duplicate - topology.hpp: take develop (macros moved to .cpp by Omri's refactor) - gda_enums.hpp: remove dead GDA_*_VENDOR_ID constants superseded by Omri's gda::vendor_id enum class - gin_anvil_sdma_factory.cc: replace (void)hipFree with checkHip() Also: - wrap_fakes.cc: add missing ncclAllToAllGinSdmaEligible stub (develop added the call in rccl_wrap.cc but no micro-test fake existed)

amd-argus

ARGUS

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

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

sdma refactors that went on concurrently

amd-argus

ARGUS

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

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

ddebonis-amd

Changes are direct remapping with no functional changes and looks correct.

omor1

We definitely want GDA and SDMA to be using the same log.hpp header.

Before approving, I also want to sync up and establish a plan for how we finish removing the link dependency on rocSHMEM and reconcile the different approaches used for GDA and SDMA.

…A plugin test stubs Co-Authored-By: Claude <noreply@anthropic.com>

device code in nccl_device, only host code in gin/log.hpp, no spillage of unqualified include in user code.

amd-argus

ARGUS

Bottom line: Nothing new worth raising; still open from earlier: CMakeLists.txt:527 and 1 more. Worth clearing before approval.

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

systems-assistant · 2026-09-29T07:34:32Z

TheRock Submodule Bump Activity

Newest first

2026-09-29 — Included in TheRock via ROCm/TheRock#8567.

Simplify build for gin-anvil and remove sdma-induced link dependencies

3cc50b8

to rocshmem (link dependencies induced by GDA remains TBD)

abouteiller requested review from a team September 17, 2026 12:39

abouteiller requested a review from a team as a code owner September 17, 2026 12:39

github-actions Bot added project: rccl project: rocshmem labels Sep 17, 2026

therock-pr-bot Bot added the Not ready to Review label Sep 17, 2026

systems-assistant Bot added the organization: ROCm label Sep 17, 2026

amd-argus Bot reviewed Sep 17, 2026

View reviewed changes

abouteiller changed the title ~~Simplify build for gin-anvil and remove sdma-induced rocshmem link dependencies~~ refactor(rccl/gin) Simplify build for gin-anvil and remove sdma-induced rocshmem link dependencies Sep 17, 2026

abouteiller changed the title ~~refactor(rccl/gin) Simplify build for gin-anvil and remove sdma-induced rocshmem link dependencies~~ refactor(rccl/gin): Simplify build for gin-anvil and remove sdma-induced rocshmem link dependencies Sep 18, 2026

amd-argus Bot reviewed Sep 18, 2026

View reviewed changes

omor1 mentioned this pull request Sep 18, 2026

refactor(rocshmem/gda): refactor QueuePair class #7217

Merged

5 of 9 tasks

amd-argus Bot reviewed Sep 21, 2026

View reviewed changes

abouteiller added 2 commits September 23, 2026 11:41

Merge remote-tracking branch 'origin/develop' into users/abouteiller/…

2e2f0aa

…rccl-gin/sdma-build-simplification

BUILT_TESTS=ON and ROCSHMEM_GIN=ON together would not compile

0a4f8f9

amd-argus Bot reviewed Sep 23, 2026

View reviewed changes

Comment thread projects/rccl/tools/ci/lib/gin.sbatch Resolved

Comment thread projects/rocshmem/src/CMakeLists.txt Resolved

Comment thread projects/rccl/CMakeLists.txt Resolved

abouteiller and others added 2 commits September 23, 2026 14:05

Update projects/rccl/tools/ci/lib/gin.sbatch

5e43005

Co-authored-by: amd-argus[bot] <321830313+amd-argus[bot]@users.noreply.github.com>

Do not reference now in-rccl symbols that are not in rocshmem anymore

63d16b9

when linking sdma

alex-breslow-amd reviewed Sep 23, 2026

View reviewed changes

Comment thread projects/rccl/src/gin/gin_anvil_sdma_factory.cc Resolved

alex-breslow-amd reviewed Sep 23, 2026

View reviewed changes

Comment thread projects/rccl/test/gin/gin_anvil_plugin_test_stubs.cc Resolved

alex-breslow-amd reviewed Sep 23, 2026

View reviewed changes

Comment thread projects/rccl/test/gin/GinAnvilPlugin_test.cpp Resolved

alex-breslow-amd approved these changes Sep 23, 2026

View reviewed changes

amd-argus Bot reviewed Sep 23, 2026

View reviewed changes

Comment thread projects/rccl/src/gin/gin_anvil_sdma_factory.cc Resolved

abouteiller added 2 commits September 24, 2026 12:05

Review comments: guard against hipfree errors

b546e37

amd-argus Bot reviewed Sep 24, 2026

View reviewed changes

Reconciliation for the build logic of the in-rccl tests between qp and

b6a7094

sdma refactors that went on concurrently

amd-argus Bot reviewed Sep 24, 2026

View reviewed changes

ddebonis-amd approved these changes Sep 24, 2026

View reviewed changes

amirsojoodi approved these changes Sep 24, 2026

View reviewed changes

omor1 requested changes Sep 24, 2026

View reviewed changes

omor1 approved these changes Sep 24, 2026

View reviewed changes

abouteiller and others added 3 commits September 24, 2026 19:20

fix(rccl/test): replace (void)hipFree with error-checked calls in SDM…

fbd200b

…A plugin test stubs Co-Authored-By: Claude <noreply@anthropic.com>

Reconciliation with qp-refactor: do not have duplicate log.hpp, only

ad4d6bd

device code in nccl_device, only host code in gin/log.hpp, no spillage of unqualified include in user code.

Use copy method for sdma files like we do for gda files

b424008

amd-argus Bot reviewed Sep 25, 2026

View reviewed changes

abouteiller merged commit efc517a into develop Sep 28, 2026
84 of 90 checks passed

abouteiller deleted the users/abouteiller/rccl-gin/sdma-build-simplification branch September 28, 2026 15:13

dlamd1dai restored the users/abouteiller/rccl-gin/sdma-build-simplification branch September 28, 2026 16:16

dlamd1dai deleted the users/abouteiller/rccl-gin/sdma-build-simplification branch September 28, 2026 16:18

dlamd1dai mentioned this pull request Sep 28, 2026

fix(rccl/gin-sdma): restore LSA conn-check with device-linked TU (AICOMRCCL-2217) #11092

Merged

6 tasks

lfmeadow mentioned this pull request Sep 28, 2026

fix(rccl): repair the --rocshmem-gin test build #12319

Closed

kuhar mentioned this pull request Sep 29, 2026

[rocjitsu] Fix floating-point and operand semantics #12467

Merged

Conversation

abouteiller commented Sep 17, 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 Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ All Policy Checks Passed

Uh oh!

therock-pr-bot Bot commented Sep 17, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abouteiller commented Sep 21, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abouteiller commented Sep 23, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alex-breslow-amd 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!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ddebonis-amd left a comment

Choose a reason for hiding this comment

Uh oh!

omor1 left a comment

Choose a reason for hiding this comment

abouteiller commented Sep 17, 2026 •

edited

Loading

therock-pr-bot Bot commented Sep 17, 2026 •

edited

Loading