refactor(rocr): abstract thunk loader platform-specific code by shadidashmiz · Pull Request #7125 · ROCm/rocm-systems

shadidashmiz · 2026-06-11T20:24:14Z

AIRUNTIME-2002
remove if defs
Abstract platform specific functions

Motivation

Remove if defs from the body of code

Technical Details

Separate Linux and Win specific functions so the thunk loader is opaque

JIRA ID: AIRUNTIME-2002

Test Plan

Hip Rocr tests

Test Result

PSDB

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

dayatsin-amd

Please change licence for header to MIT.

/*

Copyright (c) Advanced Micro Devices, Inc., or its affiliates.
SPDX-License-Identifier: MIT
*/

github-actions · 2026-07-08T00:31:45Z

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.

cfreeamd

thunk_loader.cpp, thunk_loader_linux.cpp, thunk_loader_win.cpp, and thunk_loader_platform.h look correct — behavior for all four call sites is preserved: DTIF detection, DXG detection (Linux /dev/dxg + flag, Windows unconditional), dynamic API loading, and static API binding.

therock-pr-bot · 2026-07-08T14:27:33Z

✅ 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/runtime/hsa-runtime/core/driver/kfd/amd_kfd_driver.cpp`, `projects/rocr-runtime/runtime/hsa-runtime/core/inc/thunk_loader.h`, `projects/rocr-runtime/runtime/hsa-runtime/core/runtime/amd_memory_region.cpp`, `projects/rocr-runtime/runtime/hsa-runtime/core/runtime/amd_topology.cpp`, `projects/rocr-runtime/runtime/hsa-runtime/core/runtime/runtime.cpp` (+4 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 · 2026-07-08T14:27:35Z

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

❌ PR Description

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

shadidashmiz · 2026-07-14T17:38:26Z

@cfreeamd can you please review this change as well thanks

shadidashmiz · 2026-07-21T19:12:57Z

@cfreeamd can you please review this change

shadidashmiz · 2026-07-24T14:16:31Z

@cfreeamd can you please review this change

Please change licence for header to MIT.

/*

Copyright (c) Advanced Micro Devices, Inc., or its affiliates.

SPDX-License-Identifier: MIT
*/

it is done

cfreeamd

🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after author update

Prior items resolved: license headers now use SPDX-License-Identifier: MIT (addresses the earlier CHANGES_REQUESTED), and the earlier question about DetectDxgDriver() on Windows was clarified as intentional. The refactor has since grown to also collapse is_win_dxg_/is_wsl_dxg_ into a single is_dxg_; left two follow-up notes on the resulting behavior change.

cfreeamd · 2026-07-28T02:25:58Z

 Runtime::MappedHandleAllowedAgent::~MappedHandleAllowedAgent() {
  if (targetAgent->device_type() == core::Agent::DeviceType::kAmdCpuDevice) {
-    if (core::Runtime::runtime_singleton_->thunkLoader()->IsWslDxg()) assert(!"Unimplemented");
+    if (core::Runtime::runtime_singleton_->thunkLoader()->IsDXG()) assert(!"Unimplemented");


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

[Question] This PR now collapses is_win_dxg_/is_wsl_dxg_ into a single is_dxg_ and replaces IsWinDxg()/IsWslDxg() with IsDXG(). That changes behavior at a few call sites vs. the original pure-refactor:

Here (and the other MappedHandleAllowedAgent guards) the predicate was IsWslDxg(), which was false on Windows-native DXG. It's now IsDXG(), so Windows-native DXG also takes the Unimplemented/return-error/skip path.

In amd_kfd_driver.cpp / amd_memory_region.cpp the predicate was IsWinDxg(), which was false under WSL2; it's now IsDXG(), so WSL2 now skips pinning and the per-region cap like Windows.

Both look reasonable (both paths are DXG/WDDM), but could you confirm the win-dxg/wsl-dxg merge is intentional and that no remaining caller needs to distinguish the two?

cfreeamd · 2026-07-28T02:25:58Z

    // is best-effort; WDDM will demand-page on GPU access.
    const bool is_windxg =
-        core::Runtime::runtime_singleton_->thunkLoader()->IsWinDxg();
+        core::Runtime::runtime_singleton_->thunkLoader()->IsDXG();


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

[Nit] The local is still named is_windxg but is now assigned from IsDXG(), which is also true under WSL2 (/dev/dxg). Consider renaming to is_dxg here (and in amd_memory_region.cpp) so the name matches the broadened predicate.

amd-justchen · 2026-09-01T17:45:38Z

My agent found this as a potential build failure:

TheRock CI gate fails on a genuine compile error caused by this PR's thunk-loader refactor:
amd_topology.cpp:320: error: 'const class rocr::core::ThunkLoader' has no member named 'IsWslDxg'
The refactor removed/renamed IsWslDxg but left the call site in amd_topology.cpp (a file the PR does not touch). All four Build Packages jobs fail in the compile step across every arch (gfx94X, gfx950, MI455, Windows).

Does this look related? @shadidashmiz

- remove if defs - Abstract platform specific functions Signed-off-by: sdashmiz <shadi.dashmiz@amd.com>

The refactoring unified is_win_dxg_ and is_wsl_dxg_ into a single is_dxg_ bool but amd_topology.cpp still needs to distinguish WSL DXG from native Windows DXG for tool registration. Restore IsWslDxg() using a compile-time #ifdef: on Linux is_dxg_ implies WSL, on Windows it does not. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>

Resolve thunk_loader.h against develop's thunk-loader platform split (#7125) and the GL2 residency entry point (#10183): keep this branch's nullptr-initialized table and add hsaKmtSetPersistingCacheSize to it. Bind hsaKmtSetPersistingCacheSize as an optional entry point, like hsaKmtSetSigbusDelay. librocdxg does not export it, so as a required export it would make LoadThunkApiTable() fail and hsa_init() refuse to start under WSL. The static path defaults it to the NOT_SUPPORTED stub and the Linux BindPlatformStaticApis() binds the libhsakmt implementation, which was otherwise left unbound.

systems-assistant · 2026-10-05T04:19:26Z

TheRock Submodule Bump Activity

Newest first

2026-10-05 — Included in TheRock via ROCm/TheRock#8732.

Copilot AI review requested due to automatic review settings June 11, 2026 20:24

shadidashmiz requested review from atgutier, cfreeamd, dayatsin-amd and kentrussell as code owners June 11, 2026 20:24

github-actions Bot added the project: rocr-runtime label Jun 11, 2026

Copilot started reviewing on behalf of shadidashmiz June 11, 2026 20:25

View session

Copilot AI reviewed Jun 11, 2026

systems-assistant Bot added the organization: ROCm label Jun 11, 2026

dayatsin-amd approved these changes Jun 12, 2026

View reviewed changes

Comment thread projects/rocr-runtime/runtime/hsa-runtime/core/runtime/thunk_loader_linux.cpp Resolved

dayatsin-amd self-requested a review June 12, 2026 01:46

dayatsin-amd requested changes Jun 12, 2026

View reviewed changes

shadidashmiz force-pushed the users/sdashmiz/abstract-thunk-loader branch 2 times, most recently from bb96266 to 17c5cb3 Compare June 12, 2026 14:44

github-actions Bot added the Stale PR has no activity for 25+ days label Jul 8, 2026

cfreeamd reviewed Jul 8, 2026

View reviewed changes

Comment thread projects/rocr-runtime/runtime/hsa-runtime/core/runtime/thunk_loader_win.cpp Resolved

shadidashmiz force-pushed the users/sdashmiz/abstract-thunk-loader branch from 17c5cb3 to 5d0d48e Compare July 8, 2026 14:26

therock-pr-bot Bot added the Not ready to Review label Jul 8, 2026

shadidashmiz force-pushed the users/sdashmiz/abstract-thunk-loader branch 2 times, most recently from 2022324 to 5236ef0 Compare July 8, 2026 18:35

shadidashmiz changed the title ~~AIRUNTIME-2002 - Abstract thrunk loader~~ ROCR Abstract thrunk loader by refactoring Jul 8, 2026

shadidashmiz changed the title ~~ROCR Abstract thrunk loader by refactoring~~ refactor(rocr): abstract thunk loader platform-specific code Jul 9, 2026

github-actions Bot removed the Stale PR has no activity for 25+ days label Jul 10, 2026

shadidashmiz force-pushed the users/sdashmiz/abstract-thunk-loader branch from 57403d8 to 39de11d Compare July 13, 2026 14:59

shadidashmiz requested a review from dayatsin-amd July 24, 2026 14:15

dayatsin-amd approved these changes Jul 28, 2026

View reviewed changes

cfreeamd reviewed Jul 28, 2026

View reviewed changes

cfreeamd approved these changes Jul 28, 2026

View reviewed changes

shadidashmiz requested a review from shwetagkhatri as a code owner August 20, 2026 19:31

shadidashmiz force-pushed the users/sdashmiz/abstract-thunk-loader branch from 1c71b88 to e45639c Compare September 15, 2026 19:21

AIRUNTIME-2002 - Abstract thrunk loader

0ebf3a1

- remove if defs - Abstract platform specific functions Signed-off-by: sdashmiz <shadi.dashmiz@amd.com>

shadidashmiz force-pushed the users/sdashmiz/abstract-thunk-loader branch from e45639c to 0ebf3a1 Compare September 15, 2026 19:23

shadidashmiz and others added 5 commits September 25, 2026 12:39

Merge branch 'develop' into users/sdashmiz/abstract-thunk-loader

9111294

Merge branch 'develop' into users/sdashmiz/abstract-thunk-loader

dd8e60b

Merge branch 'develop' into users/sdashmiz/abstract-thunk-loader

f81c280

Merge branch 'develop' into users/sdashmiz/abstract-thunk-loader

ef27327

amd-hsivasun merged commit 058d216 into develop Sep 30, 2026
210 of 213 checks passed

amd-hsivasun deleted the users/sdashmiz/abstract-thunk-loader branch September 30, 2026 18:37

shwetagkhatri mentioned this pull request Nov 3, 2026

fix(rocr): restrict the DXG allocation relaxations to native Windows #14954

Open

Conversation

shadidashmiz commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Test Plan

Test Result

Submission Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dayatsin-amd left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 8, 2026

Uh oh!

cfreeamd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

therock-pr-bot Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ All Policy Checks Passed

Uh oh!

therock-pr-bot Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shadidashmiz commented Jul 14, 2026

Uh oh!

shadidashmiz commented Jul 21, 2026

Uh oh!

shadidashmiz commented Jul 24, 2026

Uh oh!

cfreeamd left a comment

Choose a reason for hiding this comment

Uh oh!

cfreeamd Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

cfreeamd Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

amd-justchen commented Sep 1, 2026

Uh oh!

Uh oh!

systems-assistant Bot commented Oct 5, 2026

TheRock Submodule Bump Activity

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shadidashmiz commented Jun 11, 2026 •

edited

Loading

therock-pr-bot Bot commented Jul 8, 2026 •

edited

Loading

therock-pr-bot Bot commented Jul 8, 2026 •

edited

Loading