test(rccl): add host microtests for nccl_device/core.cc team helpers by aduforest · Pull Request #13316 · ROCm/rocm-systems

aduforest · 2026-10-12T22:03:09Z

JIRA ID: AICOMRCCL-2825

What

Host-only microtests for src/nccl_device/core.cc: the ncclTeam* views and the world/LSA rank-translation helpers. Third PR in the AICOMRCCL-2825 stack.

How

New binary rccl-UnitTestsMicroCore (core-test.cc, suite CoreMicrotest.*), because rccl-UnitTestsMicro already fakes five of core.cc's seven functions. It links the hookable ncclDevrInitOnce in fakes/dev_runtime_fakes.cc, so both its success and failure paths are tested.

Registered like every other microtest binary: run_host_tests.sh, its own test_categories_micro_core.yaml, ci-precheckin.json, and the llvm-cov list in test_executor.py.

Coverage

7/7 functions, 55/55 lines, 32/32 regions, 16/16 branches.

Mutation Testing

7 mutations on the hipified source, all killed.

Test Plan

CoreMicrotest.*: 16/16, and 16/16 × 20 shuffled.
rccl-UnitTestsMicro on develop: 1903/1903.
Test-only change, plus registering the binary in ci-precheckin.json and test_executor.py.

🤖 Generated with Claude Code

therock-pr-bot · 2026-10-12T22:04:02Z

✅ 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:04Z

@argus review

amd-argus

ARGUS

Bottom line: LGTM once TeamRail_SuccessPath uses constants that do not collide at nccl-core-test.cc:94.

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

aduforest · 2026-10-13T05:19:30Z

@argus review

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

@argus re-review

amd-argus

ARGUS

Bottom line: LGTM once TeamRail_SuccessPath's rank constant is re-picked: 10 / 4 and 10 % 4 are both 2, so core.cc:76's division is unpinned.

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

aduforest · 2026-10-13T18:00:43Z

@argus review

amd-argus

ARGUS

Findings

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

projects/rccl/test/host/MICROTEST_README.md:802 (Low) The README's two standalone-build binary lists still omit the new target The standalone-build binary list at MICROTEST_README.md:798-802 and the run block at MICROTEST_README.md:826-845 still omit rccl-UnitTestsMicroNcclDeviceCore, though CMakeLists.txt:1023 builds it on that path and both lists already carry rccl-UnitTestsMicroCe.

Bottom line: Looks approval ready; the remaining notes are all Low.

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

aduforest · 2026-10-13T19:05:10Z

@argus review

amd-argus · 2026-10-13T19:08:09Z

@aduforest the new head (fafd71e1) doesn't change any file I'd review. Looks like a merge that didn't touch this PR's own content, so nothing has changed since my last pass above; push a real change and I'll take another look.

aduforest · 2026-10-13T19:46:31Z

@argus review

amd-argus

ARGUS

Bottom line: LGTM.

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

- TeamRail_SuccessPath: avoid lsaSize^2 == nRanks so dividing by lsaSize can't be mistaken for reading it back directly. - HierMultimem/HierLsa mode tests: give the other mode's field a distinct non-zero value, so a swapped arm divides by a real (wrong) number instead of silently dividing by zero. - TeamRankToLsa: add a negative-offset case. - Fix header comment: 5 of 7 functions gate on ncclDevrInitOnce, not 6. - Drop the nccl_device EXTRA_INCLUDES entry: verified empirically (binary still builds and all tests pass without it) that core.cc does not need it; the stated reason didn't match core.cc's own include spelling.

rank=10 with lsaSize=4 gives 2 for both 10/4 and 10%4, so core.cc's rank division was unpinned. rank=9 keeps the expected 2 and makes % give 1. Co-Authored-By: Claude <noreply@anthropic.com>

…wnership wording, explicit yaml pattern Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-14T18:27:28Z

@argus review

amd-argus

ARGUS

Bottom line: LGTM.

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

…tests Shorten the MICROTEST_README entry, CMake comments and test-file comments to the length the other host microtests use. Comment and doc changes only. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-14T23:43:17Z

@argus review

Give the binary its own test_categories_micro_<stem>.yaml and register it in ci-precheckin.json and test_executor.py's llvm-cov list, as MICROTEST_README's naming section requires, so the CI test runner and coverage pick it up. Rename to core-test.cc / rccl-UnitTestsMicroCore / CoreMicrotest per the rules. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-14T23:59:51Z

@argus review

aduforest · 2026-10-15T01:11:10Z

@argus review

amd-argus

ARGUS

Bottom line: LGTM.

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

aduforest · 2026-10-15T19:27:28Z

Folded into #13312 (one PR per Jira ticket, so AICOMRCCL-2825 runs CI once). Same commits, unchanged.

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

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

aduforest mentioned this pull request Oct 12, 2026

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-core-test.cc Resolved

aduforest force-pushed the users/aduforest/aicomrccl-2825-core-host-microtests branch from 33576d9 to bb3814c Compare October 13, 2026 18:00

amd-argus Bot reviewed Oct 13, 2026

View reviewed changes

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

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

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

aduforest force-pushed the users/aduforest/aicomrccl-2825-core-host-microtests branch from bb3814c to fafd71e Compare October 13, 2026 19:05

amd-argus Bot reviewed Oct 13, 2026

View reviewed changes

aduforest marked this pull request as ready for review October 13, 2026 23:16

aduforest requested review from a team, dhanavikram and pvallem October 13, 2026 23:16

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

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

171f680

test(rccl): pick a TeamRail rank where / and % differ

efc6421

rank=10 with lsaSize=4 gives 2 for both 10/4 and 10%4, so core.cc's rank division was unpinned. rank=9 keeps the expected 2 and makes % give 1. Co-Authored-By: Claude <noreply@anthropic.com>

test(rccl): pin ncclTeamWorld/RankToWorld staying ungated, fix fake-o…

854da50

…wnership wording, explicit yaml pattern Co-Authored-By: Claude <noreply@anthropic.com>

aduforest force-pushed the users/aduforest/aicomrccl-2825-core-host-microtests branch from 253c0b5 to 854da50 Compare October 14, 2026 18:27

amd-argus Bot reviewed Oct 14, 2026

View reviewed changes

test(rccl): trim microtest docs and comments to match the other host …

9d54be4

…tests Shorten the MICROTEST_README entry, CMake comments and test-file comments to the length the other host microtests use. Comment and doc changes only. Co-Authored-By: Claude <noreply@anthropic.com>

dhanavikram approved these changes Oct 14, 2026

View reviewed changes

amd-argus Bot reviewed Oct 15, 2026

View reviewed changes

aduforest mentioned this pull request Oct 15, 2026

test(rccl): add host microtests for the device API host side #13312

Merged

aduforest closed this Oct 15, 2026

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!

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!

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 13, 2026

Uh oh!

amd-argus Bot 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!

aduforest commented Oct 14, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

aduforest commented Oct 14, 2026

Uh oh!

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

aduforest commented Oct 15, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aduforest commented Oct 12, 2026 •

edited

Loading