Skip to content

fix(clr) - hipDeviceGetLuid API fixes for WSL - #14741

Merged
erman-gurses merged 2 commits into
developfrom
users/agunashe/getluid_wsl
Nov 3, 2026
Merged

erman-gurses merged 2 commits into
developfrom
users/agunashe/getluid_wsl

Conversation

@agunashe

@agunashe agunashe commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

hipDeviceGetLuid API fixes for WSL
WSL returns valid value so make use of it

Technical Details

hipDeviceGetLuid gated on the compile-time IS_LINUX and so returned hipErrorNotSupported under WSL, even though hipGetDeviceProperties reported a real DXGI adapter LUID from the same backend — making the two APIs contradict each other and failing the cross-check test.

The gate now keys off whether the backend actually reports a LUID (luidLowPart_/luidHighPart_ non-zero), so native Linux still returns hipErrorNotSupported while WSL and Windows return the same values hipGetDeviceProperties does; the test was rewritten to assert that agreement in both directions instead of branching on _WIN32.

Issue Tracking

JIRA ID: AIRUNTIME-3125

Test Plan

hip-tests should succeed on linux and windows.
Unit_hipDeviceGetLuid_* should pass on WSL env as wel

Test Result

hip-tests should succeed on linux and windows.
Unit_hipDeviceGetLuid_* should pass on WSL env as wel

Submission Checklist

Copilot AI balanced review requested due to automatic review settings October 30, 2026 22:46
@agunashe
agunashe requested review from a team as code owners October 30, 2026 22:46
@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/clr/hipamd/src/hip_device.cpp, projects/hip-tests/catch/unit/device/hipDeviceGetLuid.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?

Copilot AI 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.

🟡 Changes recommended

The public API documentation still promises no support and untouched outputs on all non-Windows platforms, contradicting the new WSL behavior of hipDeviceGetLuid.

1 open issue
What changed in this PR

This PR enables hipDeviceGetLuid on WSL when the backend reports a valid LUID, aligning its behavior with hipGetDeviceProperties.

Changes:

  • Replace the compile-time Linux restriction with a backend LUID check.
  • Update tests to verify supported and unsupported behavior and agreement between the two APIs.
File Description
projects/​hip-tests/​catch/​unit/​device/​hipDeviceGetLuid.cc Test backend-dependent support and consistency with device properties.
projects/​clr/​hipamd/​src/​hip_device.cpp Allow LUID queries when the backend provides a non-zero LUID.

Review effort: Balanced

Comment thread projects/clr/hipamd/src/hip_device.cpp Resolved

@mangupta mangupta 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 @mangupta)

@agunashe
agunashe force-pushed the users/agunashe/getluid_wsl branch from 452b57d to e172a04 Compare November 2, 2026 20:18
@mangupta

mangupta commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

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

Re-reviewed the updated head (e172a041ff47) — no new findings at or above the posting threshold.

@erman-gurses
erman-gurses merged commit 4919ea6 into develop Nov 3, 2026
310 of 321 checks passed
@erman-gurses
erman-gurses deleted the users/agunashe/getluid_wsl branch November 3, 2026 18:45
@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.

5 participants