test(rccl): add host microtests for the device API host side by aduforest · Pull Request #13312 · ROCm/rocm-systems

aduforest · 2026-10-12T21:39:33Z

JIRA ID: AICOMRCCL-2825

What

Host-only microtests for the host side of nccl_device: the device-resource requirement builders (src/nccl_device/{cft_barrier,gin_barrier,gin_scratch,ll_a2a,lsa_barrier}.cc), the math helpers in src/include/nccl_device/utility.h, src/nccl_device/core.cc's team helpers, and src/cft_dev_runtime.cc.

How

rccl-UnitTestsMicroNcclDevice: the requirement builders, reusing the goldens from test/DeviceResourceRequirementTests.cpp; ncclCftBarrierCreateRequirement had no tests. Retiring the GPU copy is tracked in AICOMRCCL-2912.
nccl-utility-test.cc is header-only, so it goes in rccl-UnitTestsMicro.
rccl-UnitTestsMicroCore: core.cc, with the hookable ncclDevrInitOnce fake so its success and failure paths both run.
rccl-UnitTestsMicroCftDevRuntime: cft_dev_runtime.cc with core.cc compiled for real; the four dev_runtime.cc symbols it calls get fail-loud hooks in fakes/dev_runtime_fakes.cc. CUDA_VERSION is below 13030 here, so the CUDA logical-endpoint calls are compiled out.
Pins 1 production bug as a KnownBug_ test: a 1-rank team makes ncclGinInboxA2ACreateRequirement divide by zero.

Coverage

100% of lines in the builders, utility.h, core.cc and the parts of cft_dev_runtime.cc this build compiles.

Mutation Testing

46 mutations on the hipified sources, all killed.

Test Plan

Standalone build, each × 20 shuffled: rccl-UnitTestsMicroNcclDevice 14/14, NcclUtilityMicrotest.* 27/27 in rccl-UnitTestsMicro, rccl-UnitTestsMicroCore 16/16, rccl-UnitTestsMicroCftDevRuntime 35/35.

therock-pr-bot · 2026-10-12T21:40:23Z

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

aduforest · 2026-10-12T23:10:00Z

@argus review

amd-argus

ARGUS

Bottom line: Worth settling before approval: the divide-by-zero pin at nccl-device-requirements-test.cc:310 passes on any child failure, and its stated reason for avoiding a death test does not hold for this host-only binary.

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

aduforest · 2026-10-13T05:32:36Z

@argus review

aduforest · 2026-10-13T07:28:52Z

@argus re-review

amd-argus

ARGUS

Bottom line: LGTM once the stale rationale at nccl-device-requirements-test.cc:86 is reworded: it still describes the ProcessIsolatedTestRunner design this round replaced.

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

aduforest · 2026-10-13T18:32:53Z

@argus re-review

aduforest · 2026-10-13T18:58:48Z

@argus review

aduforest · 2026-10-13T22:09:29Z

@argus review

amd-argus

ARGUS

Findings

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

projects/rccl/test/host/nccl-device-requirements-test.cc:277 (Medium) The inbox handle.size_log2 is only ever read back where the expected value is 0, so zeroing the production assignment passes the whole suite handle.size_log2 is read back only at line 242, where size_log2 is 0, and bufferSize is computed from the parameter rather than the field (gin_scratch.cc:34,36), so changing gin_scratch.cc:34 to = 0; passes every assertion in this file. Device code indexes with that field (gin_scratch__types.h:134). Could this table check it too?
projects/rccl/test/host/nccl-device-requirements-test.cc:102 (Low) The new EXPECT_EXIT SIGFPE pin cannot pass under BUILD_ADDRESS_SANITIZER=ON, and this TU lacks the __asan_default_options opt-out init-test.cc carries The KilledBySignal(SIGFPE) pin at line 324 cannot match under -DBUILD_ADDRESS_SANITIZER=ON: rccl_apply_host_only_flags adds -fsanitize=address to this target (test/host/cmake/RcclTargetHelpers.cmake:84, applied at test/host/CMakeLists.txt:403), and ASan handles SIGFPE itself. init-test.cc:148-151 carries the opt-out for the same reason. Could this TU do the same?

Bottom line: Worth settling before approval: the inbox size_log2 readback gap at nccl-device-requirements-test.cc:277, where setting gin_scratch.cc:34 to = 0; would still pass the whole suite.

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

- rollingLessEq: add a partial-width (nBits=32 on a uint64_t) pair; every existing call passed nBits == full width, leaving the mask shift dead. - idivFast64: add a quotient above 2^32 (1<<40 / 1); the existing case's quotient fit in 32 bits, so the truncation it claimed to test was inert. - AlignUp/AlignDown pointer overload: pin exact addresses instead of mod/bound checks that an off-by-one in the production formula also passes. - roundUp/roundDown: add a non-power-of-two modulus pair (3), since modulus-8-only can't tell % from a power-of-two mask. - idivRcp32/64 sweeps: add 0xFFFFFFFFu / ~0ull to catch an over-estimating reciprocal, which the existing +1 under-estimate correction can't. - Remove a tautological loop in the rolling-comparator test: it reduced to the same call on both sides regardless of a swap bug, so it could not fail. - Fix two comments: the IdivRcp32 pow2-correction is a branch-avoidance optimisation, not required for round-trip correctness; and the 'no HIP/device-only dependency' header/README claim doesn't hold for the four memory_order helpers (true only because this build's --offload-host-only path still invokes hipcc). - Two self-referential assertions (mul32hi/mul64hi) replaced with literal expected values. - present_opt -> presentOpt naming nit.

…wrapper sweeps Co-Authored-By: Claude <noreply@anthropic.com>

…tops.h using-declaration trap Co-Authored-By: Claude <noreply@anthropic.com>

…dary, and imulRcp's rem negation Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-14T03:03:36Z

@argus review

…rement builders

- Replace the ProcessIsolatedTestRunner-based divide-by-zero pin with a real EXPECT_EXIT(..., KilledBySignal(SIGFPE), "") death test: this binary is always --offload-host-only, so GTEST_HAS_DEATH_TEST is true here and the old comment's reason for avoiding one didn't hold. EXPECT_FALSE(executeAllTests()) passed on any child failure, not only SIGFPE; the new test pins the exact signal and can no longer pass on a fork failure, timeout, or unrelated crash. - Build the crashing team's nRanks from a volatile int so the divide can't be constant-folded away by an optimizing build. - Drop the now-unused ProcessIsolatedTestRunner include and CMake source entry (nothing else in this binary used it). - LLA2ACalcSlots_LinearInMaxElts: add a (2,4) row distinguishing maxElts*divUp(maxEltSize,8) from divUp(maxElts*maxEltSize,8), which (5,8) alone could not. - Note AICOMRCCL-2912 (filed) for retiring the duplicate GPU-binary copy in test/DeviceResourceRequirementTests.cpp once this suite is trusted in CI. - Nits: rename the floor-clamp test/vector for the at-or-below-7 case, fix include order.

…s fork-style death test Co-Authored-By: Claude <noreply@anthropic.com>

… row, literal alignment goldens Co-Authored-By: Claude <noreply@anthropic.com>

…comrccl-2825-device-requirements-host-microtests

aduforest · 2026-10-16T17:11:14Z

@argus review

amd-argus

ARGUS

This PR changes 1,896 lines across 14 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.}

The four dev_runtime.cc hooks now live in fakes/dev_runtime_fakes.cc and fail loud by default. Drop the six symBindTeamLe/symUnbindTeamLe tests that cannot fail on this build, and name the divide-by-zero pin KnownBug_. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-16T21:27:27Z

@argus review

amd-argus

ARGUS

This PR changes 1,802 lines across 14 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.}

aduforest · 2026-10-16T22:04:42Z

@argus re-review

amd-argus · 2026-10-16T22:07:16Z

@aduforest the current head (cf0a98af) 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.

aduforest · 2026-10-16T22:29:55Z

@argus review

amd-argus · 2026-10-16T22:35:13Z

@aduforest the current head (cf0a98af) 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.

# Conflicts: # projects/rccl/test/host/MICROTEST_README.md

aduforest · 2026-10-19T17:26:35Z

@argus review

amd-argus

ARGUS

This PR changes 1,837 lines across 14 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.}

# Conflicts: # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/tools/scripts/test_runner/lib/test_executor.py

aduforest · 2026-10-19T17:53:44Z

@argus review

amd-argus

ARGUS

This PR changes 1,827 lines across 14 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-2825-device-requirements-host-microtests Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-20T12:50:30Z

@argus review

amd-argus

ARGUS

This PR changes 1,795 lines across 13 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 · 2026-10-21T05:46:14Z

@aduforest Hi, a heads-up: NcclDeviceRequirementsMicrotest.GinInboxA2ACreateRequirement_KnownBug_SingleRankTeamDividesByZero, added here, fails with "failed to die" in MathCI precheckin(rccl) on gfx942 and gfx1100 (seen on #12751). I can reproduce it on current develop (02d6cbc711), building rccl-UnitTestsMicroNcclDevice with ROCm 7.0.2 amdclang++ in Release.

The likely cause is that the test #includes gin_scratch.cc, so ncclGinInboxA2ACreateRequirement can be inlined. Its idivRcp32(0) result is never read afterward, and division by zero is undefined behavior, so the optimizer can drop the div and no SIGFPE fires. Reading handle.nPeers_rcp32 through a volatile sink inside the death-test body might keep the divide, though only a check that doesn't rely on undefined behavior would be robust.

Could you take a look? Thanks!

systems-assistant · 2026-10-21T13:32:26Z

TheRock Submodule Bump Activity

Newest first

2026-10-21 — Included in TheRock via ROCm/TheRock#9398.

JIRA ID: AICOMRCCL-3122 ## What Fixes the math-ci precheckin(rccl) failure `NcclDeviceRequirementsMicrotest.GinInboxA2ACreateRequirement_KnownBug_SingleRankTeamDividesByZero` ("failed to die"), from #13312. It's red on every RCCL PR, for example #12751. ## How Removes that death test and its now-unused core-dump helper. The test relied on undefined behaviour: it needed x86_64 to trap on a divide by zero and the compiler to keep a divide whose result was never used. math-ci's optimized build drops the divide, so nothing trapped. The production bug (a 1-rank team divides by zero in `ncclGinInboxA2ACreateRequirement`) stays tracked in AICOMRCCL-3121. A fix there should add a normal test that a 1-rank team is rejected. ## Coverage Unchanged: the line is still covered by the other gin_scratch tests. ## Mutation Testing N/A, test removal. ## Test Plan `rccl-UnitTestsMicroNcclDevice` 13/13 in both Release and Debug standalone builds, 20 shuffled repeats clean in each. The Release build reproduced the math-ci failure before this change. --------- Co-authored-by: Claude <noreply@anthropic.com>

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

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

This was referenced Oct 12, 2026

test(rccl): add host microtests for nccl_device/utility.h math helpers #13315

Closed

test(rccl): add host microtests for nccl_device/core.cc team helpers #13316

Closed

test(rccl): add host microtests for src/cft_dev_runtime.cc #13320

Closed

amd-argus Bot reviewed Oct 13, 2026

View reviewed changes

aduforest mentioned this pull request Oct 13, 2026

test(rccl): add host microtests for the ROCm runtime and SMI wrappers #13335

Merged

amd-argus Bot reviewed Oct 13, 2026

View reviewed changes

Comment thread projects/rccl/test/host/nccl-device-requirements-test.cc Resolved

aduforest force-pushed the users/aduforest/aicomrccl-2825-device-requirements-host-microtests branch from 2d00b59 to babf51a Compare October 13, 2026 18:30

aduforest force-pushed the users/aduforest/aicomrccl-2825-device-requirements-host-microtests branch from babf51a to fbe9fb3 Compare October 13, 2026 19:02

amd-argus Bot reviewed Oct 14, 2026

View reviewed changes

aduforest and others added 5 commits October 13, 2026 20:00

test(rccl): add host microtests for nccl_device/utility.h math helpers

f81f407

test(rccl): pin alignUp's - 1, rollingLessThan's nBits forward, trim …

47a1255

…wrapper sweeps Co-Authored-By: Claude <noreply@anthropic.com>

test(rccl): correct the imulRcp32 counterexample comment, note the bi…

d1eb14c

…tops.h using-declaration trap Co-Authored-By: Claude <noreply@anthropic.com>

test(rccl): pin alignDown's negation, rollingLessEq's half-range boun…

87d7310

…dary, and imulRcp's rem negation Co-Authored-By: Claude <noreply@anthropic.com>

aduforest force-pushed the users/aduforest/aicomrccl-2825-device-requirements-host-microtests branch from fbe9fb3 to 7ebc682 Compare October 14, 2026 03:03

aduforest and others added 5 commits October 14, 2026 11:09

test(rccl): add host microtests for nccl_device device-resource requi…

f8e4b96

…rement builders

test(rccl): fix DisableCoreDumpsForExpectedCrash rationale for gtest'…

68b9e90

…s fork-style death test Co-Authored-By: Claude <noreply@anthropic.com>

test(rccl): non-identity team rank/stride, an inexact-reciprocal peer…

f6d697c

… row, literal alignment goldens Co-Authored-By: Claude <noreply@anthropic.com>

test(rccl): add host microtests for nccl_device/core.cc team helpers

171f680

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

345aff3

…comrccl-2825-device-requirements-host-microtests

amd-argus Bot reviewed Oct 16, 2026

View reviewed changes

Merge remote-tracking branch 'origin/develop' into agent/2825-pr

207d55f

# Conflicts: # projects/rccl/test/host/MICROTEST_README.md

amd-argus Bot reviewed Oct 19, 2026

View reviewed changes

Merge remote-tracking branch 'origin/develop' into agent/2825-pr

6201da0

# Conflicts: # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/tools/scripts/test_runner/lib/test_executor.py

amd-argus Bot reviewed Oct 19, 2026

View reviewed changes

aduforest and others added 2 commits October 19, 2026 23:42

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

5ee4212

…comrccl-2825-device-requirements-host-microtests Co-Authored-By: Claude <noreply@anthropic.com>

Merge remote-tracking branch 'origin/develop' into agent/2825-pr

6608366

amd-argus Bot reviewed Oct 20, 2026

View reviewed changes

aduforest merged commit 41301ca into develop Oct 20, 2026
71 of 78 checks passed

aduforest deleted the users/aduforest/aicomrccl-2825-device-requirements-host-microtests branch October 20, 2026 14:52

dlamd1dai mentioned this pull request Oct 21, 2026

fix(gin): de-alias Anvil signals across logical contexts #12751

Merged

1 task done

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

aduforest mentioned this pull request Oct 21, 2026

test(rccl): drop the nccl_device divide-by-zero death test #13927

Merged

Conversation

aduforest commented Oct 12, 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!

therock-pr-bot Bot commented Oct 12, 2026

✅ All Policy Checks Passed

Uh oh!

aduforest commented Oct 12, 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!

aduforest commented Oct 13, 2026

Uh oh!

aduforest commented Oct 13, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aduforest commented Oct 13, 2026

Uh oh!

aduforest commented Oct 13, 2026

Uh oh!

aduforest commented Oct 13, 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!

aduforest commented Oct 14, 2026

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 commented Oct 16, 2026

Uh oh!

aduforest commented Oct 16, 2026

Uh oh!

amd-argus Bot commented Oct 16, 2026

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!

aduforest commented Oct 12, 2026 •

edited

Loading