fix(rccl): probe fabric APIs with plain compiler in topo_expl by mch · Pull Request #11657 · ROCm/rocm-systems

mch · 2026-09-15T18:42:57Z

The topo_expl tool bypasses RCCL's CMake configuration and reproduces the HIP/amdsmi fabric feature probes in its own Makefile. Those probes piped a snippet through hipcc's HIP driver mode. On recent ROCm toolchains hipcc's HIP driver pulls in cuda_wrappers/bits/c++config.h, which clashes with a recent system libstdc++ (redeclaration of __glibcxx_assert_fail). That makes the otherwise-valid probe fail; the '&& echo yes || echo no' guard swallows the error and returns 'no', so AMDSMI_FABRIC_DIRECT is left undefined and amdsmi_wrap.h compiles its compat copies of the UALoE fabric types. On base images whose amd_smi/amdsmi.h now defines those types natively (ROCm 10.2.0 / TheRock main), the compat definitions collide with the real header and the topo_expl target fails to build, breaking the whole extra build. The main librccl build was unaffected because CMake probes with the plain compiler, which does not go through the HIP driver.

This is not PR-specific: it fails identically on extra/rccl/develop and multiple unrelated PRs since the math-ci base-image bump to TheRock main.

Fix: probe the fabric APIs the same way CMake does — compile a real temp file with the plain compiler (amdclang++, overridable via PROBE_CXX) instead of piping through hipcc's HIP driver. Both probes now agree with the CMake build.

Testing: verified locally that the old hipcc probe returns 'no' (reproducing the misfire) and the new plain-compiler probe returns 'yes' for both the HIP and amdsmi fabric probes; make -p confirms FEATURE_DEFINES now includes -DAMDSMI_FABRIC_DIRECT.

JIRA ID : AICOMRCCL-2359

therock-pr-bot · 2026-09-15T18:43: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/tools/topo_expl/src/topo_expl_impl.cpp`; 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

ARGUS

Bottom line: Worth settling before approval: PROBE_CXX at Makefile:12 has no existence check, so a missing amdclang++ silently answers no from both probes and re-creates the amdsmi_wrap.h collision this PR fixes.

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

amd-argus

ARGUS

Findings

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

projects/rccl/tools/topo_expl/Makefile:53 (Low) PROBE_TMP has no emptiness guard, so a failed mktemp makes both probe commands unparseable and drops both fabric defines PROBE_TMP gets no emptiness guard, so a failing mktemp turns both probe commands into a redirection with no target (> && $(PROBE_CXX) ...), which the shell refuses to parse. HAVE_HIP_FABRIC_API and HAVE_AMDSMI_FABRIC_API then come back empty instead of no, AMDSMI_FABRIC_DIRECT stays undefined, and the compat type collision returns with only a bare shell syntax error to explain it. Same guard shape as lines 14-16:

Bottom line: LGTM once the PROBE_TMP emptiness guard at Makefile:53 is added; that one is mechanical. The PROBE_CXX executability guard at Makefile:17 rewrites a conditional, so it is worth another look.

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

amd-argus

ARGUS

Bottom line: LGTM once the PROBE_TMP emptiness guard at Makefile:54 is added. Both findings this round are mechanical, so they do not need another pass from me.

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

amd-argus

ARGUS

Bottom line: LGTM. 1 finding(s) below Medium not posted this round; comment @argus re-review full for a full pass if you want them.

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

…_expl The topo_expl tool bypasses RCCL's CMake configuration and reproduces the HIP/amdsmi fabric feature probes in its own Makefile. Those probes piped a snippet through hipcc, i.e. hipcc's HIP driver mode. On recent ROCm toolchains hipcc's HIP driver pulls in cuda_wrappers/bits/c++config.h, which clashes with a recent system libstdc++ (redeclaration of __glibcxx_assert_fail). That makes the otherwise-valid probe fail to compile; the '&& echo yes || echo no' guard swallows the error and returns 'no', so AMDSMI_FABRIC_DIRECT is left undefined. With AMDSMI_FABRIC_DIRECT unset, amdsmi_wrap.h compiles its compat copies of the UALoE fabric types. On base images whose amd_smi/amdsmi.h now defines those types natively (ROCm 10.2.0 / TheRock main), the compat definitions collide with the real header and the topo_expl target fails to build, breaking the whole extra build. The main librccl build was unaffected because CMake probes with the plain compiler (check_cxx_source_compiles), which does not go through the HIP driver and correctly detected the API. Probe the fabric APIs the same way CMake does: compile a real temp file with the plain compiler (amdclang++, overridable via PROBE_CXX) instead of piping through hipcc's HIP driver. Both probes now agree with the CMake build, so AMDSMI_FABRIC_DIRECT is set correctly and the compat block is skipped when the real header provides the fabric API. Co-authored-by: Pi Agent

…tion Locate the probe compiler across bin/ and llvm/bin/ like toolchain-linux.cmake and fail loudly when none is executable, so a missing compiler no longer degrades a probe to "no". Gate each probe compile on a successful temp-file write and assign the cleanup shell call to a variable. Co-authored-by: Pi Agent

The previous guard tested existence via $(wildcard) and nested the command -v check, so a PROBE_CXX that exists but is not runnable skipped the check and the not-executable error never fired. Resolve PROBE_CXX and test -x on the resolved path so the exists-but-not-runnable case is caught. Co-authored-by: Pi Agent

…cm_smi topo_expl still included rocm_smi/rocm_smi.h and defined legacy rocm_smi_* discovery stubs, while the main RCCL build long ago migrated to amd_smi and forces USE_AMDSMI ON. On newer base images (ROCm >= 10.2 / TheRock main) the rocm_smi header no longer exists, so once the fabric-probe fix let the compile advance past the UALoE compat-type collision it failed with 'rocm_smi/rocm_smi.h file not found'. Align topo_expl with the library build: define USE_AMDSMI in the Makefile so the staged xml.cc/rccl_wrap.cc take the amd_smi discovery path, replace the rocm_smi include with amdsmi_wrap.h, and swap the rocm_smi_* stubs for the amd_smi_* entry points those sources call (init, getNumDevice, getDevicePciBusIdString, getDeviceIndexByPciBusId, getLinkInfo, getFabricDeviceInfo, getFirmwareVersion). The stubs report "no devices" so model-driven discovery falls through exactly as before. Testing: with rocm_smi/rocm_smi.h hidden to mimic the CI base image, all translation units now compile (-fsyntax-only exit 0) whereas pristine develop reproduces the original failure. Co-authored-by: Pi Agent

amd-argus

ARGUS

Bottom line: 4 new finding(s) below High not posted this round (@argus re-review full for a full pass); still open from earlier: Makefile:54. Worth clearing before approval.

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

… guards Treat an explicitly empty PROBE_CXX override as unset so detection still runs, and fail loudly if mktemp cannot create the feature-probe temp file instead of silently degrading both probes. Co-authored-by: Pi Agent

mch · 2026-09-17T14:18:49Z

@argus re-review full

amd-argus

ARGUS

Bottom line: LGTM.

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

alex-breslow-amd

LGTM. Thank you sir for the fix.

systems-assistant · 2026-09-19T03:58:37Z

TheRock Submodule Bump Activity

Newest first

2026-09-19 — Included in TheRock via ROCm/TheRock#8334.

Extra gfx90a compile failed because tests still used the pre-2.31 CE arity and a removed GDR flush inject helper. ## Motivation The NCCL 2.31 sync changed CE and GDR test APIs. Unit tests on develop still used the old signatures, causing extra’s gfx90a compile failed. The extra issues were: - CE tests — ncclCeAvailable / ncclHierCeAvailable still called with 5 args; 2.31 needs 7. - GDR tests — gdrSupported() is gone; RunRecvFlushBurst no longer takes forceWrite. - topo_expl / amdsmi — fabric enums redefined vs ROCm 7.0.2; already probed on develop (#11657), not in this PR. - DDA duplicate helper — already gone on develop. ## Technical Details - Pass `nullptr, nullptr` into `ncclCeAvailable` / `ncclHierCeAvailable` so calls match the 7-arg 2.31 API (`sendWin`, `recvWin`). - Match the host CE fake `ncclHierCeAvailable` in `ce_fakes.cc` to that 7-arg signature. - Replace `gdrSupported()` with `gdrPtrSupport()` and stop calling the removed 4-arg `RunRecvFlushBurst(..., forceWrite, ...)`. - `topo_expl` / amdsmi fabric-enum redefinition is already handled on `develop` by the Makefile probe in #11657; this PR does not change that path. ## Issue Tracking JIRA ID: AICOMRCCL-1982 Follow-up to #10785 (extra gfx90a compile). ## Test Plan - Compile `CeAlltoAllvEligibilityTests.cpp`, `GdrFlushTests.cpp`, and `ce_fakes.cc` for gfx90a (`--offload-arch=gfx90a`, `ENABLE_FAULT_INJECTION`) against the 2.31 headers. - Confirm unpatched `CeAlltoAllvEligibilityTests.cpp` still fails with extra’s `no matching function for call to 'ncclHierCeAvailable'` / `ncclCeAvailable`. - After merge, extra: Compile ubuntu24-therock && gfx90a (including `topo_expl`). ## Test Result - gfx90a object compile succeeded for the three patched TUs in Docker (`rocm/dev-ubuntu-22.04:7.2`). - Unpatched CE test still reproduced extra’s 5-vs-7-arg errors. - Extra gfx90a / `topo_expl` on the internal ROCm 7.0.2 image is not re-run in this PR until extra CI picks up the branch. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

The topo_expl tool bypasses RCCL's CMake configuration and reproduces the HIP/amdsmi fabric feature probes in its own Makefile. Those probes piped a snippet through hipcc's HIP driver mode. On recent ROCm toolchains hipcc's HIP driver pulls in cuda_wrappers/bits/c++config.h, which clashes with a recent system libstdc++ (redeclaration of __glibcxx_assert_fail). That makes the otherwise-valid probe fail; the '&& echo yes || echo no' guard swallows the error and returns 'no', so AMDSMI_FABRIC_DIRECT is left undefined and amdsmi_wrap.h compiles its compat copies of the UALoE fabric types. On base images whose amd_smi/amdsmi.h now defines those types natively (ROCm 10.2.0 / TheRock main), the compat definitions collide with the real header and the topo_expl target fails to build, breaking the whole extra build. The main librccl build was unaffected because CMake probes with the plain compiler, which does not go through the HIP driver. This is not PR-specific: it fails identically on extra/rccl/develop and multiple unrelated PRs since the math-ci base-image bump to TheRock main. Fix: probe the fabric APIs the same way CMake does — compile a real temp file with the plain compiler (amdclang++, overridable via PROBE_CXX) instead of piping through hipcc's HIP driver. Both probes now agree with the CMake build. Testing: verified locally that the old hipcc probe returns 'no' (reproducing the misfire) and the new plain-compiler probe returns 'yes' for both the HIP and amdsmi fabric probes; `make -p` confirms FEATURE_DEFINES now includes -DAMDSMI_FABRIC_DIRECT. JIRA ID : AICOMRCCL-2359

mch requested review from a team September 15, 2026 18:42

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

amd-argus Bot reviewed Sep 15, 2026

View reviewed changes

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

amd-argus Bot reviewed Sep 15, 2026

View reviewed changes

Comment thread projects/rccl/tools/topo_expl/Makefile Resolved

amd-argus Bot reviewed Sep 15, 2026

View reviewed changes

Comment thread projects/rccl/tools/topo_expl/Makefile Resolved

Comment thread projects/rccl/tools/topo_expl/Makefile Resolved

nileshnegi approved these changes Sep 15, 2026

View reviewed changes

amd-argus Bot reviewed Sep 16, 2026

View reviewed changes

mch added 4 commits September 16, 2026 23:18

mch force-pushed the users/mahughes/aicomrccl-2359-topo-expl-probe branch from 9d3c623 to eccab55 Compare September 16, 2026 23:20

amd-argus Bot reviewed Sep 17, 2026

View reviewed changes

mch force-pushed the users/mahughes/aicomrccl-2359-topo-expl-probe branch from cc3d58b to cc701d6 Compare September 17, 2026 14:18

amd-argus Bot reviewed Sep 17, 2026

View reviewed changes

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

View reviewed changes

mch merged commit 2a98fd6 into develop Sep 17, 2026
43 of 47 checks passed

mch deleted the users/mahughes/aicomrccl-2359-topo-expl-probe branch September 17, 2026 16:04

amd-jiali mentioned this pull request Sep 17, 2026

fix(rccl): match CE and GDR unit tests to NCCL 2.31 signatures #11797

Merged

1 task

Conversation

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

Uh oh!

Uh oh!

therock-pr-bot Bot commented Sep 15, 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 left a comment

Choose a reason for hiding this comment

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

Findings

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

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!

mch commented Sep 17, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

alex-breslow-amd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

systems-assistant Bot commented Sep 19, 2026

TheRock Submodule Bump Activity

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mch commented Sep 15, 2026 •

edited

Loading

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

edited

Loading