Skip to content

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

Open
benrichard-amd wants to merge 1 commit into
developfrom
users/benrichard-amd/rccl-xml-test-x86-guard
Open

benrichard-amd wants to merge 1 commit into
developfrom
users/benrichard-amd/rccl-xml-test-x86-guard

Conversation

@benrichard-amd

@benrichard-amd benrichard-amd commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Part of work for ROCm/TheRock#5518

xml-test.cc (added in #13605) unconditionally includes <cpuid.h> and uses __cpuid() x86 inline asm, causing a compile error on non-x86 architectures. Two tests also hardcode EXPECT_EQ("x86_64", ...) for the arch attribute, which fails on ppc64le where the SUT correctly returns "ppc64".

Technical Details

  • Guard #include <cpuid.h>, the CpuId struct, HostCpuId() function, and the GetXmlFromCpu_FillsAffinityArchAndCpuid test behind #if defined(__x86_64__) || defined(_M_X64), matching the convention used in xml.cc, init.cc, and init-test.cc.
  • Add a NCCL_HOST_ARCH preprocessor macro that mirrors the arch detection logic in src/graph/xml.cc ("x86_64" / "ppc64" / "arm64"). Replace the two remaining hardcoded "x86_64" assertions in GetXmlFromSys_NoSysfsAttachesToTheUnknownCpu and GetXmlFromSys_ExistingParentIsFilledInTurn with NCCL_HOST_ARCH.
  • The GetXmlFromCpu_FillsAffinityArchAndCpuid test remains x86-only since it validates cpuid vendor/family/model fields that only exist on x86.

Test Plan

Built RCCL and ran the host microtest suites (ctest -R "Micro|HostOnly|MicroTopo") on a POWER10 ppc64le machine.

Test Result

137/153 ctest targets pass. The 2 previously failing XmlSysfsMicrotest arch tests now pass. The 16 remaining failures (2 unique tests × suite/config variants) are pre-existing and unrelated:

  • P2pSchedule_ZeroGroupSizeParam_DiesOnDivideByZero — death test expects SIGFPE, but POWER hardware returns 0 for integer divide-by-zero instead of trapping.
  • GroupJobUnlinkCommsTest.EmptyAsyncJobsStillClearsTheCommHeadChain — groupJob pointer not being cleared; logic bug in the new test.

@benrichard-amd
benrichard-amd requested review from a team October 27, 2026 21:01
@therock-pr-bot

Copy link
Copy Markdown

✅ 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/xml-test.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?

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Findings

3 more finding(s), not posted inline
  • projects/rccl/test/host/xml-test.cc:658 (Low) x86 guard wraps the whole of GetXmlFromCpu_FillsAffinityArchAndCpuid, so off x86 nothing asserts the non-zero affinity_offset branch (xml.cc:529); guard only the cpuid parts.
  • projects/rccl/test/host/xml-test.cc:18 (Nit) NCCL_HOST_ARCH tests PPC64 variants where xml.cc:535 tests PPC; use PPC so the test copy matches production.
  • projects/rccl/test/host/xml-test.cc:17 (Nit) Two adjacent blocks repeat the same x86 guard; fold the <cpuid.h> include into the x86_64 branch of the NCCL_HOST_ARCH chain.

Bottom line: LGTM.

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

Comment thread projects/rccl/test/host/xml-test.cc Resolved
Comment thread projects/rccl/test/host/xml-test.cc Resolved
Comment thread projects/rccl/test/host/xml-test.cc Resolved
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>
@benrichard-amd
benrichard-amd force-pushed the users/benrichard-amd/rccl-xml-test-x86-guard branch from 6a7284e to 742ce26 Compare November 3, 2026 17:05

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Bottom line: LGTM.

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

@benrichard-amd
benrichard-amd requested a review from mch November 4, 2026 15:40

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants