Skip to content

test(rccl): fake the rocSHMEM host symbols the microtests need - #12337

Merged
lfmeadow merged 3 commits into
developfrom
users/lmeadows/rocshmem-host-test-fakes
Sep 29, 2026
Merged

lfmeadow merged 3 commits into
developfrom
users/lmeadows/rocshmem-host-test-fakes

Conversation

@lfmeadow

Copy link
Copy Markdown
Contributor

JIRA ID : LCOMPILER-2794

./install.sh -t --rocshmem fails to link four host test binaries. init.cc and enqueue.cc reach their ENABLE_ROCSHMEM arms, and nothing in fakes/ defines the ten symbols those arms call. Each fake goes in the file named after the owning production TU; rocshmem_fakes.cc is new and conditional, since it includes rocshmem.hpp.

Pre-existing, but unreachable under ninja until #12331 let --rocshmem start building.

Test

  • -t --ninja --rocshmem --address-sanitizer: 3197/3197, was 3 link failures
  • -t --ninja: 4366/4366, unchanged

@lfmeadow
lfmeadow requested review from a team and a lite review from Copilot September 28, 2026 03:43
@lfmeadow

Copy link
Copy Markdown
Contributor Author

@argus re-review

@therock-pr-bot

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

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/fakes/bootstrap_stubs.cc, projects/rccl/test/host/fakes/comm_fakes.cc, projects/rccl/test/host/fakes/hip_fakes.cc, projects/rccl/test/host/fakes/hip_fakes.h, projects/rccl/test/host/fakes/init_fakes.cc (+3 more); 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?

This comment was marked as resolved.

amd-argus[bot]

This comment was marked as outdated.

@lfmeadow
lfmeadow force-pushed the users/lmeadows/rocshmem-host-test-fakes branch from de91c03 to f267d94 Compare September 28, 2026 17:41
amd-argus[bot]

This comment was marked as outdated.

./install.sh -t --rocshmem fails to link four host test binaries: init.cc and
enqueue.cc reach their ENABLE_ROCSHMEM arms, and nothing in fakes/ defines the
symbols those arms call. Adds them, in the fakes file named after the owning
production TU.

rocshmem_fakes.cc is conditional -- it includes rocshmem.hpp, which is only on
the include path under ENABLE_ROCSHMEM.

Verified with -t --ninja --rocshmem --address-sanitizer (3197/3197, was 3 link
failures) and -t --ninja (4366/4366, unchanged).
The test targets resolved <rocshmem/rocshmem.hpp> from ${ROCM_PATH}/include,
a different rocSHMEM from the one librccl.so links, and nothing ordered them
after the sub-build that installs rccl's copy. Put ROCSHMEM_INCLUDE_DIR first
and add the ordering.

Also drops a build log committed by accident, registers ResetRocshmemFakes in
the Init reset chain, adds the third result seam, and removes the standalone
source-list branch, which never defines ENABLE_ROCSHMEM.
…view

The three seams take the calls' own parameters, with a Default* each and a
drift check against the real declarations. rocshmem_malloc's state moves to
file scope so ResetRocshmemFakes clears it. The rocSHMEM include directory
goes just ahead of ${ROCM_PATH}/include rather than first, and two comments
are corrected.
@lfmeadow
lfmeadow force-pushed the users/lmeadows/rocshmem-host-test-fakes branch from f267d94 to c09b80b Compare September 28, 2026 19:55
@lfmeadow

Copy link
Copy Markdown
Contributor Author

@argus re-review

@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.

@alex-breslow-amd alex-breslow-amd 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.

LGTM

@lfmeadow
lfmeadow merged commit 9c02ecd into develop Sep 29, 2026
83 of 87 checks passed
@lfmeadow
lfmeadow deleted the users/lmeadows/rocshmem-host-test-fakes branch September 29, 2026 22:27
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

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.

4 participants