test(rccl): add host microtests for src/graph by aduforest · Pull Request #13605 · ROCm/rocm-systems

aduforest · 2026-10-15T22:32:36Z

JIRA ID: AICOMRCCL-2799

What

Host-only microtests for src/graph: xml.cc/xml.h, topo.cc/topo.h, paths.cc, search.cc, rome_models.cc, connect.cc, rings.cc and trees.cc, in a new rccl-UnitTestsMicroTopo binary (482 tests, about 5.5 s).

How

One test TU per graph file, each #includeing the hipified unit through <STEM>_CC_PATH.
Own binary: rccl-UnitTestsMicroGraph links tuning_fakes.cc, which defines connect.cc's params, and os_fakes.cc, whose CPU and sysfs seams these tests cannot drive. HIP and amd-smi go through fakes/hip_fakes.cc and fakes/amdsmi_fakes.cc; the other externals are in fakes/graph_fakes.{h,cc}.
Topologies are NCCL_TOPO_FILE-style XML built in TopoScene.h and parsed by the real xml.cc/topo.cc, so paths, search, the Rome model matchers and connect run on real input.
Pins 10 production bugs as KnownBug_ tests (AICOMRCCL-3024..3031):
- ncclTopoAddCpu: the Rome/Zen classification overwrites the Zen generation, so AMD SYS bandwidth is always AMD_ZEN12_BW.
- ncclTopoGetXmlCpuArch: no arch on any <cpu> dereferences NULL.
- ncclTopoAddGpu: a <gpu> without gcn segfaults.
- ncclTopoAutoMerge: after a failed merge device 0 is never re-placed.
- ncclTopoFillGpu: the static amdsmiInit guard is never set, so amd_smi_init runs on every call.
- parseA2a8P: 8 GPUs with 16 NICs walk about 15! NIC orderings, stalling init.
- parseA2a8P and parse1H16P leak all_gpu_permutations when no model matches.
- parseGraph writes gpus[gpu] one past the stack array before checking the count.
- parseOptions drops option pairs past the fifth (latent).
Seen but not pinned: ncclTopoAutoMerge's "too many NICs" return skips free(paths).

Coverage

8323/8397 lines (99.12%) over the hipified graph sources, with develop's rccl-UnitTestsMicroRomeModels covering parseGraphLight; what's left is allocation failures, dead code and branches compiled out under HIP.

Mutation Testing

272 targeted mutants, 270 killed, 2 equivalent.

Test Plan

rccl-UnitTestsMicroTopo (standalone build): 482 passed, and --gtest_shuffle --gtest_repeat=20 is clean; rccl-UnitTestsMicroConnect and rccl-UnitTestsMicroRomeModels still pass.

New binary rccl-UnitTestsMicroTopo compiles the real src/graph files, one test TU each, against fakes/graph_fakes.cc. TopoScene.h builds topology XML in the test so the real parser and system builder run host-only. Co-Authored-By: Claude <noreply@anthropic.com>

Covers building the system from XML (XGMI, MLOPart, C2C, BCM switches, NICs), NIC fusion and plugin import, local NIC/GPU queries, ncclTopoGetSystem, and path computation with the P2P/GDR/PXN decisions. Co-Authored-By: Claude <noreply@anthropic.com>

…ls.cc search-test.cc drives ncclTopoCompute over XML-built systems (rings, trees, NVLS, CollNet, inter-node NIC selection, graph XML import/export) and the search helpers directly. rome-models-test.cc rebuilds every Rome model's signature from XML and runs each matcher (4P2H, A2a8P, 1H16P, 4H4P, GigaIO, chordal ring) plus the ring/tree string parsers. paths-test.cc gains ncclTopoTrimSystem and ncclTopoGetPxnRanks now that search.cc links. Co-Authored-By: Claude <noreply@anthropic.com>

…d trees.cc connect-test.cc covers ncclTopoPreset, the tree-string postset, ring and tree wiring across nodes (plain, load-balanced and rail-optimized), CollNet/NVLS heads, channel-count limits and ncclTopoPostset end to end. rings-test.cc and trees-test.cc cover ring linearization and the binary and double binary trees. With connect.cc linked, paths-test.cc gains the ncclTopoComputeP2pChannels tests. Long lines wrapped to 120 columns. Co-Authored-By: Claude <noreply@anthropic.com>

The ncclTopoRankToIndex/DevToRank/IdToNetDev test now checks the warn flag and that the NIC dev comes from the plugin, not the node index. The parseGraph host-limit test is split: the GPU-count case writes one entry past gpus[] before the bound check, so it is pinned as observed and skipped under AddressSanitizer. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-15T22:33:14Z

@argus review

therock-pr-bot · 2026-10-15T22:33:35Z

✅ 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

Findings

5 more finding(s), not posted inline

projects/rccl/test/host/rome-models-test.cc:739 (Low) ParseGraph_OneGpuPastTheLimitIsWrittenBeforeTheStop runs a real out-of-bounds stack write in the main gtest process
projects/rccl/test/host/TopoScene.h:57 (Low) TempFile ignores a mkstemp failure, keeps the fixed template path, and later reads, writes and unlinks it
projects/rccl/test/host/fakes/graph_fakes.h:8 (Low) graph_fakes.h rationale says the shared fakes define graph symbols, but hip_fakes.cc, amdsmi_fakes.cc, os_fakes.cc and bootstrap_stubs.cc define none
projects/rccl/test/host/search-test.cc:30 (Low) GraphPtr is defined in three test files and NewGraph twice near-identically; both belong in TopoScene.h
projects/rccl/test/host/rome-models-test.cc:441 (Low) Outer loops wrapping nested multi-line bodies are unbraced (rome-models-test.cc:271, 280, 287, 441; topo-test.cc:61), against nccl_coding_style.md

This PR changes 9,273 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: mallinfo2() in rome-models-test.cc needs glibc 2.33, two tests put a multi-MB ncclComm on the stack, and AutoMerge_OverfullVnicIsAnError leaks under LeakSanitizer.

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

- InUseBytes falls back to mallinfo() before glibc 2.33. - The parent ncclComm in two tests moves to the heap. - AutoMerge_OverfullVnicIsAnError pins the leaked path matrix and skips under ASan; the parseGraph overflow runs in a death-test child. - TempFile reports a mkstemp failure instead of using the template path. - GraphPtr/NewGraph and the ASan switch move to TopoScene.h. - Braces on every control statement whose body spans lines. - DumpLine test passed fewer values than ranks; it now passes all 12. - graph_fakes.h names the shared fakes that define graph symbols. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-15T23:43:19Z

Non-inline findings, all fixed in e15a22f:

rome-models-test.cc:739: the parseGraph overflow pin now runs the call inside an EXPECT_EXIT child (threadsafe style), so the out-of-bounds write never runs in the gtest process. It still skips under ASan.
TopoScene.h:57: TempFile reports ADD_FAILURE() << "mkstemp failed" and leaves the path empty, and the destructor only unlinks a real path.
graph_fakes.h:8: the comment now names the shared fakes that define graph symbols (topo_stubs.cc, nccl_fakes.cc, init_fakes.cc, tuning_fakes.cc). It also says why the HIP, amd-smi, OS and bootstrap seams live here: one ResetGraphFakes() covers every hook.
search-test.cc:30: GraphPtr and NewGraph now live only in TopoScene.h, along with the ASan switch the leak pins use.
Unbraced loops: I fixed every control statement in the PR whose body spans lines (46 sites, including the five listed), not just the listed ones.

aduforest · 2026-10-15T23:43:21Z

@argus re-review

amd-argus

ARGUS

This PR changes 9,334 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: LGTM once REMOVE_DEFS ENABLE_WARP_SPEED is added to the rccl-UnitTestsMicroTopo call in test/host/CMakeLists.txt, so the binary links in a warp-speed (gfx950) in-tree build.

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

With ENABLE_WARP_SPEED from an in-tree build, connect.cc calls rcclGetMaxWarpsPerBlock and rcclWarpSpeedComputeNChannels, which only rccl_wrap.cc defines, so the binary would not link. Drop the define as rccl-UnitTestsMicroInit does. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-16T00:07:40Z

@argus re-review

amd-argus

ARGUS

This PR changes 9,335 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: LGTM.

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

…icrotests develop added its own connect-test.cc for rccl-UnitTestsMicroConnect (#12679). Keep it as is and move this PR's connect.cc tests to graph-connect-test.cc, with suites renamed GraphConnect*. connect.cc now records the preset channel count and expands it in Postset, so the Postset fixture sets nChannels and the intra tree/CollNet chain rows, and the new code gets its own tests. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-16T15:10:24Z

@argus review

amd-argus

ARGUS

This PR changes 9,398 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: LGTM.

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

…icrotests develop added rccl-UnitTestsMicroRomeModels with its own rome-models-test.cc. Keep it as is and move this PR's rome_models.cc tests to graph-rome-models-test.cc, with suites renamed GraphRomeModels*. Drop the two parseGraphLight tests that develop's suite now covers. The registration files (README, run_host_tests.sh, ci-precheckin.json, test_executor.py) keep both binaries. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-16T16:38:06Z

@argus review

amd-argus

ARGUS

This PR changes 9,379 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: LGTM.

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

… KnownBug_ pins - Compute_NvlsBacksOffWhenTheUplinkIsShort asserts the reduced bandwidth; the search tests whose names claimed more than nChannels >= 1 now check the NICs, channel counts or bandwidth they name, and FollowPath's check is unconditional. - graph_fakes.cc: hipGetDevice, bootstrapAllGather and canConnect default to FailLoudUnfaked; the hand-written params are pinned with rccl_assert_source_line; the header says why the owner fakes files aren't used. - The real own-binary reason in CMake and the README, a "Where a fake belongs" row, and a reason for REMOVE_DEFS ENABLE_WARP_SPEED. - Pinned production bugs are named <Fn>_KnownBug_<What>; the amdsmiInit pin moves out of the FillGpu happy path. AutoMerge_OverfullVnicIsAnError no longer claims to pin the leak. - Drop the Postset checks develop's ConnectMicrotest.PresetPostset_* already make. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-16T21:32:33Z

@argus review

amd-argus

ARGUS

This PR changes 9,442 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: LGTM.

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

…icrotests #3894 adds the compact double tree (NCCL_TREE_COMPACT) and a useCompactTree argument to getTreeNodeParity. The parity test covers both modes, and new tests pin ncclGetDtreeCompact's shape and its use in connectTrees, connectNvls and the rail-optimized trees. #13536 makes the XML gpu dev the SMI index and a failed HIP lookup an error, so the xml and ncclTopoGetSystem tests follow. #12336's MLOPart sibling channel count and single-node round fitting get tests. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-17T01:29:45Z

@argus review

amd-argus

ARGUS

This PR changes 9,588 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: LGTM.

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

…comrccl-2799-graph-host-microtests # Conflicts: # projects/rccl/test/host/CMakeLists.txt # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/test/host/run_host_tests.sh # projects/rccl/tools/scripts/test_runner/configs/ci-precheckin.json # projects/rccl/tools/scripts/test_runner/lib/test_executor.py

aduforest · 2026-10-19T18:03:57Z

@argus review

amd-argus

ARGUS

This PR changes 9,618 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: LGTM.

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

…o binary - graph_fakes.cc no longer redefines the hip_fakes.cc and amdsmi_fakes.cc hooks (same names, same signatures); it links both and ResetGraphFakes() sets the graph defaults on them. amdsmi_fakes.cc gains the init-call counter. - Hooks installed mid-test use ScopedHook, and env changes in test bodies use RcclUnitTesting::ScopedEnv. - Death tests restore death_test_style. - KnownBug_ pins name their Jira ticket. - test_executor.py lists only this PR's binary. Co-Authored-By: Claude <noreply@anthropic.com>

…comrccl-2799-graph-host-microtests

aduforest · 2026-10-19T20:55:02Z

@argus review

amd-argus

ARGUS

This PR changes 9,540 lines across 19 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.}

…comrccl-2799-graph-host-microtests Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-20T17:02:01Z

@argus review

amd-argus

ARGUS

This PR changes 9,540 lines across 19 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.}

systems-assistant · 2026-10-22T19:13:21Z

TheRock Submodule Bump Activity

Newest first

2026-10-22 — Included in TheRock via ROCm/TheRock#9425.

xml-test.cc (added in #13605) unconditionally includes <cpuid.h> and uses __cpuid() inline asm, which fails on non-x86 architectures (ppc64le, aarch64). Guard the include, the HostCpuId() helper, and the one test that calls it behind an x86 preprocessor check. Two other tests hardcode EXPECT_EQ("x86_64", ...) for the arch attribute, but the SUT correctly returns the host arch (e.g. "ppc64"). Add a constexpr kHostArch that mirrors the arch detection in xml.cc (same condition order: __PPC__, __aarch64__, x86; same macro names) so these tests pass on any architecture. Unknown architectures produce a hard #error to surface the contract immediately. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest and others added 5 commits October 15, 2026 15:14

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

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

amd-argus Bot reviewed Oct 15, 2026

View reviewed changes

Comment thread projects/rccl/test/host/rome-models-test.cc Resolved

Comment thread projects/rccl/test/host/paths-test.cc Resolved

Comment thread projects/rccl/test/host/topo-test.cc Resolved

amd-argus Bot reviewed Oct 16, 2026

View reviewed changes

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

amd-argus Bot reviewed Oct 16, 2026

View reviewed changes

aduforest marked this pull request as ready for review October 16, 2026 01:52

aduforest requested review from a team October 16, 2026 01:52

amd-argus Bot reviewed Oct 16, 2026

View reviewed changes

amd-argus Bot reviewed Oct 17, 2026

View reviewed changes

amd-argus Bot reviewed Oct 19, 2026

View reviewed changes

aduforest and others added 2 commits October 19, 2026 13:54

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

c6d4b98

…comrccl-2799-graph-host-microtests

amd-argus Bot reviewed Oct 19, 2026

View reviewed changes

aduforest and others added 2 commits October 20, 2026 08:42

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

564d6d4

…comrccl-2799-graph-host-microtests Co-Authored-By: Claude <noreply@anthropic.com>

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

a74c957

…comrccl-2799-graph-host-microtests Co-Authored-By: Claude <noreply@anthropic.com>

amd-argus Bot reviewed Oct 20, 2026

View reviewed changes

dhanavikram approved these changes Oct 20, 2026

View reviewed changes

pvallem approved these changes Oct 20, 2026

View reviewed changes

aduforest merged commit 4ca8623 into develop Oct 20, 2026
71 of 76 checks passed

aduforest deleted the users/aduforest/aicomrccl-2799-graph-host-microtests branch October 20, 2026 20:50

This was referenced Oct 21, 2026

[rocjitsu] Correct legacy floating min/max and median selection #13898

Merged

[rocjitsu] Expose backed CDNA3 and CDNA4 hardware identity fields #13899

Merged

[rocjitsu] Implement scratch LDS transfers and scalar cache discard #13900

Merged

thomas-huber mentioned this pull request Oct 22, 2026

test(rccl): pin the NIC gate on the graph search even-channel preference #14002

Draft

1 task done

benrichard-amd mentioned this pull request Oct 27, 2026

fix(rccl): guard x86 cpuid and arch assertions in xml-test.cc #14413

Open

Conversation

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

Uh oh!

What

How

Coverage

Mutation Testing

Test Plan

Uh oh!

aduforest commented Oct 15, 2026

Uh oh!

therock-pr-bot Bot commented Oct 15, 2026

✅ All Policy Checks Passed

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduforest commented Oct 15, 2026

Uh oh!

aduforest commented Oct 15, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aduforest commented Oct 16, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

aduforest commented Oct 16, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

aduforest commented Oct 16, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

aduforest commented Oct 16, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

aduforest commented Oct 17, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

aduforest commented Oct 19, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

aduforest commented Oct 19, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

aduforest commented Oct 20, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

systems-assistant Bot commented Oct 22, 2026

TheRock Submodule Bump Activity

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

aduforest commented Oct 15, 2026 •

edited

Loading