fix(rccl): repair the --rocshmem-gin test build by lfmeadow · Pull Request #12319 · ROCm/rocm-systems

lfmeadow · 2026-09-26T01:14:06Z

JIRA ID : LCOMPILER-2794

The rocSHMEM tests were missing required dependencies. ./install.sh -t --rocshmem-gin fails with 99 compile errors and produces no test binaries;
librccl.so itself is fine.

Test targets that do not link rccl inherit neither the
include/nccl_device/gin/rocshmem_gda include directory nor the ordering edge
against copy_rocshmem_headers, yet they still reach those staged headers
through nccl_device.h. This adds both, plus a missing
ncclAllToAllGinSdmaEligible fake.

The second commit folds the ten copies of that ordering guard into
rccl_order_rocshmem_headers().

Notes for review

One of five independent PRs replacing the closed #9763 and #12221. Orthogonal to
the other four.

Verified by ./install.sh -t --ninja --rocshmem-gin: 0 errors, with the ordering
edges confirmed in the ninja graph for all six affected targets.

Update after review

Review caught one more target that needed the same ordering edge,
rccl-UnitTestsMicroTaskPrep. Its task_posttuning.cc reaches
gda/queue_pair_provider.hpp via dev_runtime.h -> nccl_device.h ->
impl/gin__funcs.h -> gin/gin_device_api.h, because
NCCL_GIN_ROCSHMEM_GDA_ENABLE defaults to 1 under ENABLE_ROCSHMEM_GIN
(gin_device_common.h:53-57) and the target's NCCL_GIN_ANVIL_SDMA_ENABLE=0
closes only the anvil_sdma branch. That makes seven affected targets, and the
verification run above predates this addition.

I audited the rest for the same gap and found none. The test targets that
neither link rccl nor appear in RCCL_TEST_EXECUTABLES --
rccl-UnitTestsNetTelemetry, rccl-UnitTestsGinSdmaPolicy,
rccl-UnitTestsGinSdmaAllGatherPolicy, rccl-UnitTestsGinSdmaAllGatherGpu and
rccl-UnitTestsGinSdmaBroadcastPolicy -- include only
${PROJECT_SOURCE_DIR}/src/include and never receive
RCCL_COMMON_COMPILE_DEFS, so ENABLE_ROCSHMEM_GIN is never defined for them.

The explicit guard on rccl-UnitTestsGinAnvilPlugin is also dropped: that target
is in RCCL_TEST_EXECUTABLES, which the new foreach already covers.

therock-pr-bot · 2026-09-26T01:14:52Z

✅ 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/test/host/fakes/wrap_fakes.cc`; 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?

Copilot

Copilot review overview

🟡 Changes recommended

Add rocSHMEM header-ordering dependencies for rccl-UnitTestsMicroTaskPrep in both affected CMake target definitions.

Review effort: Lite
Findings: 2

Open (2)

Order rocSHMEM headers before MicroTaskPrep compilation · New
Add rocSHMEM header dependency to MicroTaskPrep target · New

What changed in this PR

Repairs the rocSHMEM-GIN test build by staging required headers, enforcing build ordering, and adding a missing fake. Two host micro-test targets still lack ordering dependencies.

Changes:

Adds centralized rocSHMEM header-ordering logic.
Applies GDA include paths and ordering to test and device-linker targets.
Adds ncclAllToAllGinSdmaEligible fake.

File	Description
`projects/rccl/test/host/fakes/wrap_fakes.cc`	Adds the missing all-to-all GIN fake.
`projects/rccl/test/host/CMakeLists.txt`	Adds ordering for host micro-tests.
`projects/rccl/test/CMakeLists.txt`	Adds GDA includes and test ordering.
`projects/rccl/CMakeLists.txt`	Includes the shared helper.
`projects/rccl/cmake/RocshmemHeaders.cmake`	Centralizes header-copy ordering.
`projects/rccl/cmake/DeviceLinker.cmake`	Uses the shared ordering helper.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

amd-argus

ARGUS

Bottom line: Looks approval ready once the open rccl-UnitTestsMicroTaskPrep thread is settled.

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

amd-argus

ARGUS

Bottom line: Looks approval ready. The rccl-UnitTestsMicroTaskPrep edge from the last round is in; the one remaining note is a CI-coverage follow-up, not a change to this PR.

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

lfmeadow · 2026-09-26T20:13:12Z

@argus re-review

Copilot

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified.

Review effort: Lite
Findings: None

Resolved since last review (2)

Add rocSHMEM header dependency to MicroTaskPrep target
Order rocSHMEM headers before MicroTaskPrep compilation

amd-argus · 2026-09-26T20:19:24Z

@lfmeadow the current head (8a6c6d9e) already has a review from me above; nothing has changed since then for me to look at again. Push a change and I'll pick it up, or ask again once you have.

A GIN build (`./install.sh -t --rocshmem-gin`) failed with 99 compile errors and never produced the test binaries. librccl.so itself was fine; every failure was in test/ and test/host/. Two independent causes, the second only reachable once the first is fixed. Include path. The rocSHMEM GDA headers are staged into include/nccl_device/gin/rocshmem_gda/ and include each other as "gda/...", so that directory has to be on the include path itself, not just its parent. rccl adds it (src/CMakeLists.txt) and re-exports it as an INTERFACE include directory, so test targets that link rccl inherit it. rccl-UnitTestsDevRuntime and the host micro-tests deliberately do not link rccl, yet still reach those headers through nccl_device.h, so they got neither -- 'gda/queue_pair_provider.hpp' file not found. Add it to RCCL_COMMON_INCLUDE_DIRS under ENABLE_ROCSHMEM_GIN, which covers both the test/ loop and the host micro-tests, and order those targets after copy_rocshmem_headers so they cannot race the build-time copy. That ordering covers all six host micro-test targets, including rccl-UnitTestsMicroTaskPrep: its task_posttuning.cc reaches gda/queue_pair_provider.hpp through dev_runtime.h -> nccl_device.h -> impl/gin__funcs.h -> gin/gin_device_api.h, since NCCL_GIN_ROCSHMEM_GDA_ENABLE defaults to 1 under ENABLE_ROCSHMEM_GIN and the target's NCCL_GIN_ANVIL_SDMA_ENABLE=0 closes only the anvil branch. The new ordering in the test/ loop also subsumes the existing inline guard on rccl-UnitTestsGinAnvilPlugin, which is in RCCL_TEST_EXECUTABLES, so that guard is removed. Missing fake. With the headers resolving, rccl-UnitTestsMicro then failed to link: rcclSelectAlltoAll calls ncclAllToAllGinSdmaEligible under ENABLE_ROCSHMEM_GIN, whose real definition lives in the device TU gin_alltoall_sdma.cu that this host-only binary does not compile. wrap_fakes.cc already stubs the two AllReduce GIN-SDMA entry points for exactly this reason; add the alltoall one alongside them, returning false so the selector keeps its non-GIN behaviour. Verified with clean builds both ways: --rocshmem-gin now completes with zero failures (was 99), and a plain build is unchanged.

Ten sites repeated the same three-line guard to order a target after copy_rocshmem_headers, seven of them added by the previous commit. The pattern is easy to get wrong by omission rather than by error: leaving it out does not fail the configure, it makes the target race the header copy, so the build succeeds or fails on 'gda/...' not found depending on scheduling. That is exactly how the test targets came to be missing it. Replace them with rccl_order_rocshmem_headers(), which absorbs both guards (GIN off, and target not yet created by src/) so call sites carry none, and gives the requirement one place to be explained.

amd-argus

ARGUS

Bottom line: LGTM.

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

lfmeadow · 2026-09-28T19:56:11Z

Closing as superseded: develop has since fixed the --rocshmem-gin -t test build its own way (#12231, #11757), and the repeated header-ordering blocks this PR folded into a helper are gone. A clean ./install.sh -t --ninja --rocshmem-gin --amdgpu_targets gfx950 of develop plus #12331 builds with no errors.

lfmeadow requested review from a team and a lite review from Copilot September 26, 2026 01:14

github-actions Bot added the project: rccl label Sep 26, 2026

Copilot started reviewing on behalf of lfmeadow September 26, 2026 01:14

View session

Copilot AI reviewed Sep 26, 2026

View reviewed changes

Comment thread projects/rccl/test/CMakeLists.txt Resolved

Comment thread projects/rccl/test/host/CMakeLists.txt Resolved

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

amd-argus Bot reviewed Sep 26, 2026

View reviewed changes

Comment thread projects/rccl/test/CMakeLists.txt Resolved

lfmeadow force-pushed the users/lmeadows/fix-gin-test-includes branch from b8d5339 to 8a6c6d9 Compare September 26, 2026 18:50

amd-argus Bot reviewed Sep 26, 2026

View reviewed changes

Comment thread projects/rccl/test/CMakeLists.txt Resolved

This was referenced Sep 26, 2026

fix(rccl): forward inherited device options to the dispatcher compile #12318

Merged

fix(rccl): carry the full GPU target ID through the device-linker pipeline #12313

Closed

lfmeadow requested a lite review from Copilot September 26, 2026 20:13

Copilot started reviewing on behalf of lfmeadow September 26, 2026 20:14

View session

Copilot AI reviewed Sep 26, 2026

View reviewed changes

lfmeadow force-pushed the users/lmeadows/fix-gin-test-includes branch from 8a6c6d9 to dc9b434 Compare September 26, 2026 20:44

lfmeadow added 2 commits September 27, 2026 20:53

lfmeadow force-pushed the users/lmeadows/fix-gin-test-includes branch from dc9b434 to 8455f44 Compare September 28, 2026 01:53

amd-argus Bot reviewed Sep 28, 2026

View reviewed changes

lfmeadow mentioned this pull request Sep 28, 2026

fix(rccl): carry the full GPU target ID through the device-linker pipeline #12394

Merged

lfmeadow closed this Sep 28, 2026

Navigation Menu

Repository navigation

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

fix(rccl): repair the --rocshmem-gin test build#12319
lfmeadow wants to merge 2 commits into
developfrom
users/lmeadows/fix-gin-test-includes

lfmeadow commented Sep 26, 2026 •

edited

Loading

Uh oh!

therock-pr-bot Bot commented Sep 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

amd-argus Bot left a comment

Uh oh!

Uh oh!

amd-argus Bot left a comment

Uh oh!

Uh oh!

lfmeadow commented Sep 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

amd-argus Bot commented Sep 26, 2026

Uh oh!

amd-argus Bot left a comment

Uh oh!

lfmeadow commented Sep 28, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lfmeadow commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes for review

Update after review

Uh oh!

therock-pr-bot Bot commented Sep 26, 2026

✅ All Policy Checks Passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

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!

lfmeadow commented Sep 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

amd-argus Bot commented Sep 26, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

lfmeadow commented Sep 28, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lfmeadow commented Sep 26, 2026 •

edited

Loading