Skip to content

refactor(rocrtst): remove hwloc dependency from memory async copy tests - #8723

Merged
amd-hsivasun merged 4 commits into
developfrom
users/dayatsin/remove-rocrtst-hwloc
Oct 1, 2026
Merged

amd-hsivasun merged 4 commits into
developfrom
users/dayatsin/remove-rocrtst-hwloc

Conversation

@dayatsin-amd

@dayatsin-amd dayatsin-amd commented Jul 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Drop the mandatory hwloc build/link dependency from rocrtst by using HSA_AGENT_INFO_NODE for NUMA topology in Memory_Async_Copy instead of hwloc PCIe lookup.
  • Remove vendored hwloc headers/libs and the hwloc CMake discovery block.
  • Switch the optional NUMA async-copy test (ENABLE_COPY_NUMA) to libnuma APIs when enabled.
  • Add a simple-topology fallback when no GPU shares a CPU NUMA node (restores prior single-node behavior and avoids the old hwloc/OpenCL shutdown hack).

JIRA ID

JIRA ID : ROCM-1282

Test plan

  • Build rocrtst64 on dayatsin-test-01 (gfx1100, /opt/rocm) without hwloc
  • Build rocrtst64 on e3-2b (gfx950, TheRock toolchain) without hwloc
  • Run rocrtstPerf.Memory_Async_Copy and rocrtstPerf.Memory_Async_Copy_On_Engine on both systems — 2/2 passed on each host

@therock-pr-bot

therock-pr-bot Bot commented Jul 16, 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/rocr-runtime/rocrtst/suites/performance/memory_async_copy.cc, projects/rocr-runtime/rocrtst/suites/performance/memory_async_copy.h, projects/rocr-runtime/rocrtst/suites/performance/memory_async_copy_numa.cc, projects/rocr-runtime/rocrtst/suites/performance/memory_async_copy_numa.h, projects/rocr-runtime/rocrtst/suites/performance/memory_async_copy_on_engine.cc (+1 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?

@therock-pr-bot

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ Unit Test

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

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

🤖 AI-assisted pre-review (for @cfreeamd)

Reviewed the ROCr-scoped changes (rocrtst memory_async_copy). The hwloc removal, CMake cleanup, and dropping the old hwloc hsa_shut_down()/hsa_init() teardown hack in Close() all look correct. One correctness concern on the NUMA local/remote classification left inline.

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been inactive for 25 days and will be marked as stale.

If you would like to keep this PR open, please:

  • Add new commits
  • Add a comment explaining why it should remain open

This PR will be automatically closed in 5 days if no further activity occurs.

@github-actions github-actions Bot added the Stale PR has no activity for 25+ days label Aug 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically closed due to inactivity (30 days with no updates).

If you'd like to continue working on this, feel free to reopen the PR or create a new one.

@github-actions github-actions Bot closed this Aug 18, 2026
@cfreeamd cfreeamd reopened this Sep 28, 2026
dayatsin-amd and others added 2 commits September 28, 2026 15:44
Use HSA_AGENT_INFO_NODE for NUMA topology instead of hwloc PCIe lookup,
drop vendored hwloc headers/libs, and switch the optional NUMA test to libnuma.

Co-authored-by: Cursor <cursoragent@cursor.com>
If no GPU shares a NUMA node with any CPU agent, assign the first CPU and
GPU agents so Memory_Async_Copy can run on single-socket systems.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cfreeamd
cfreeamd force-pushed the users/dayatsin/remove-rocrtst-hwloc branch from 1ebcfeb to 78a98bd Compare September 28, 2026 20:48
@cfreeamd cfreeamd removed Stale PR has no activity for 25+ days Not ready to Review labels Sep 28, 2026
HSA_AGENT_INFO_NODE returns the KFD topology node id, which is unique per
agent, so a CPU and a GPU never share it. The local-vs-remote comparison in
Memory_Async_Copy was therefore always unequal: the "local GPU" branch was
dead and classification always fell through to the iteration-order fallback,
silently losing the H2D-vs-H2DRemote distinction.

Read the GPU's OS NUMA node from its PCI numa_node in sysfs and compare it
against the CPU agent's NUMA node, restoring the real locality test that the
removed hwloc PCI-ancestor lookup provided. No hwloc dependency is
reintroduced.

Co-Authored-By: Claude <noreply@anthropic.com>
@amd-hsivasun
amd-hsivasun merged commit 2a17c1c into develop Oct 1, 2026
8 of 9 checks passed
@amd-hsivasun
amd-hsivasun deleted the users/dayatsin/remove-rocrtst-hwloc branch October 1, 2026 23:10
@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