Skip to content

fix(clr): honor no-preference LDS carveout; fix hipDeviceGetCacheConfig - #13807

Merged
iassiour merged 3 commits into
developfrom
users/iassiour/clr-lds-carveout-no-preference
Nov 1, 2026
Merged

iassiour merged 3 commits into
developfrom
users/iassiour/clr-lds-carveout-no-preference

Conversation

@iassiour

@iassiour iassiour commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

The extended AQL dispatch packet's group_mem_carveout hint uses 0 for no preference and [1, 127] for 0%..100% group memory (LDS). CLR never sends 0, so every dispatch without a preference asks for 0% LDS. Related problems:

  • hipFuncCachePreferNone is stored as an explicit 0%, so it overrides hipDeviceSetCacheConfig.
  • hipFuncCachePreferL1 maps to 1% (encoding 2) instead of 0% (1).
  • hipDeviceGetCacheConfig always returns hipFuncCachePreferNone.
  • hipDeviceReset does not clear the device-level carveout.

Technical Details

  • Cache-config table is {None: -1, Shared: 100, L1: 0, Equal: 50}; -1 means no preference.
  • The device hint is a single std::atomic<int8_t> (default -1). hipDeviceGetCacheConfig maps it back, and hipDeviceReset clears it.
  • amd::encodeGroupMemCarveout: -1 -> 0, otherwise (percent + 1) * 126 / 100, which matches the previous encoding for all of 0..100.
  • Dispatch uses the kernel percent, falling back to the device hint (both may be -1). The AIRUNTIME-2610 - implement hipKernelSetCacheConfig() #12628 hipKernelSetCacheConfig override and the per-ISA 127 override are unchanged.
  • The extended AQL dispatch log prints group_mem_carveout (AMD_LOG_LEVEL=5 AMD_LOG_MASK=8).
  • CHANGELOG and hipDeviceSetCacheConfig/hipDeviceGetCacheConfig doxygen updated.
  • Tests: VerifyCarveoutMapping expectations updated, new Unit_hipDeviceGetCacheConfig_Positive_RoundTrip, and an RAII guard restores the device cache config in the device tests.

Issue Tracking

JIRA ID: AIRUNTIME-2976

Test Plan

  • Cache-config device and executionControl tests on gfx950 (no carveout support), after the rebase on AIRUNTIME-2610 - implement hipKernelSetCacheConfig() #12628.
  • group_mem_carveout captured from the dispatch log on a simulation model of a carveout-capable target, develop vs this PR (before the rebase).
  • encodeGroupMemCarveout compared with develop for every percentage 0..100.

Test Result

All tests pass; explicit percentages encode as on develop.

Scenario develop this PR
No kernel or device preference 1 0
hipFuncSetCacheConfig(PreferL1) 2 1
Device PreferShared, function PreferNone 1 127
Device PreferNone, attribute -1 1 0
Device PreferEqual, then hipDeviceReset 64 0
hipDeviceGetCacheConfig after PreferShared PreferNone PreferShared

Submission Checklist

@therock-pr-bot

therock-pr-bot Bot commented Oct 20, 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/clr/hipamd/src/hip_device.cpp, projects/clr/hipamd/src/hip_device_runtime.cpp, projects/clr/rocclr/device/device.hpp, projects/clr/rocclr/device/rocm/rocvirtual.cpp, projects/hip-tests/catch/unit/device/hipDeviceSetGetCacheConfig.cc (+2 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:

  • ❌ PR Description

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

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

Shared cache state lacks synchronization, and the round-trip test accepts the previous always-no-preference behavior.

2 open issues
What changed in this PR

This PR corrects HIP cache-preference handling in CLR, distinguishing no preference from explicit zero-percent LDS carveout and reporting device cache configuration.

Changes:

  • Correct carveout mapping and dispatch encoding.
  • Store, report, and reset device cache preferences.
  • Update mapping tests and add round-trip coverage.
File Description
projects/​hip-tests/​catch/​unit/​executionControl/​hipFuncSetCacheConfig.cc Correct expected carveout mappings.
projects/​hip-tests/​catch/​unit/​device/​hipDeviceSetGetCacheConfig.cc Add round-trip checks and restore defaults.
projects/​hip-tests/​catch/​config/​configs/​unit/​device.yaml Register the round-trip test.
projects/​clr/​rocclr/​device/​rocm/​rocvirtual.cpp Encode and log dispatch carveout preferences.
projects/​clr/​rocclr/​device/​device.hpp Preserve no-preference values and define encoding.
projects/​clr/​hipamd/​src/​hip_internal.hpp Store device cache configuration.
projects/​clr/​hipamd/​src/​hip_device.cpp Reset cache and carveout preferences.
projects/​clr/​hipamd/​src/​hip_device_runtime.cpp Implement device cache configuration reporting.

Review effort: Balanced

@iassiour
iassiour marked this pull request as ready for review October 25, 2026 20:28
@iassiour
iassiour requested review from a team as code owners October 25, 2026 20:28
@iassiour
iassiour force-pushed the users/iassiour/clr-lds-carveout-no-preference branch from 084fa5a to 3f3ac63 Compare October 25, 2026 22:01
@iassiour
iassiour requested a balanced review from Copilot October 25, 2026 22:43

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.

🟢 Approval recommended

The signed preference handling, atomic storage, getter, reset, and dispatch encoding are consistent, with no unresolved blocking findings.

No open issues 🎉

2 resolved since last review

Review effort: Balanced

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

[SUGGESTION] Document the changed behavior of hipDeviceGetCacheConfig and the cache-config-to-carveout mapping

This PR changes what users can observe through the public API. (1) hipDeviceGetCacheConfig used to return hipFuncCachePreferNone every time. It now returns the last value passed to hipDeviceSetCacheConfig on devices that support a configurable carveout. (2) After hipFuncSetCacheConfig, the preferredShmemCarveout value seen by callers is now -1 for hipFuncCachePreferNone (was 0) and 0 for hipFuncCachePreferL1 (was 1). (3) hipDeviceReset now clears the device carveout hint. (4) A 'no preference' setting no longer forces a 0%-LDS dispatch encoding. The diff contains no CHANGELOG entry and no doxygen update for any of these.

Suggested fix: Add a projects/clr/CHANGELOG.md entry that covers: the LDS no-preference fix, hipDeviceGetCacheConfig reporting the configured value (with PreferNone on devices that have no configurable carveout), the new preferredShmemCarveout values (-1 and 0), and hipDeviceReset restoring the default. If the hipDeviceGetCacheConfig and hipDeviceSetCacheConfig doxygen in hip_runtime_api.h says the setting is ignored or that PreferNone is always returned on AMD, update it.

@iassiour
iassiour force-pushed the users/iassiour/clr-lds-carveout-no-preference branch from 3f3ac63 to 7f2a5bd Compare October 26, 2026 16:25
@iassiour
iassiour requested review from a team as code owners October 26, 2026 16:25
@iassiour
iassiour requested a balanced review from Copilot October 26, 2026 20:43

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 encoder changes 24 explicit-percentage mappings despite the stated compatibility guarantee, requiring preservation or explicit documentation and testing.

1 open issue

Review effort: Balanced

Comment thread projects/clr/rocclr/device/device.hpp Resolved
@iassiour

Copy link
Copy Markdown
Contributor Author

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

[SUGGESTION] Document the changed behavior of hipDeviceGetCacheConfig and the cache-config-to-carveout mapping

This PR changes what users can observe through the public API. (1) hipDeviceGetCacheConfig used to return hipFuncCachePreferNone every time. It now returns the last value passed to hipDeviceSetCacheConfig on devices that support a configurable carveout. (2) After hipFuncSetCacheConfig, the preferredShmemCarveout value seen by callers is now -1 for hipFuncCachePreferNone (was 0) and 0 for hipFuncCachePreferL1 (was 1). (3) hipDeviceReset now clears the device carveout hint. (4) A 'no preference' setting no longer forces a 0%-LDS dispatch encoding. The diff contains no CHANGELOG entry and no doxygen update for any of these.

Suggested fix: Add a projects/clr/CHANGELOG.md entry that covers: the LDS no-preference fix, hipDeviceGetCacheConfig reporting the configured value (with PreferNone on devices that have no configurable carveout), the new preferredShmemCarveout values (-1 and 0), and hipDeviceReset restoring the default. If the hipDeviceGetCacheConfig and hipDeviceSetCacheConfig doxygen in hip_runtime_api.h says the setting is ignored or that PreferNone is always returned on AMD, update it.

addressed in 7f2a5bd

  • projects/clr/CHANGELOG.md ## Unreleased: Resolved issues covers the no-preference dispatch fix (including PreferNone no longer overriding hipDeviceSetCacheConfig) and hipDeviceGetCacheConfig returning the configured value; Changed covers the new preferredShmemCarveout readback (-1 for PreferNone, 0 for PreferL1).
  • hip_runtime_api.h: the hipDeviceSetCacheConfig/hipDeviceGetCacheConfig doxygen no longer says AMD ignores the setting or always returns PreferNone. The getter note says fixed-carveout devices report hipFuncCachePreferNone, and the setter note says hipDeviceReset() restores the default.

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.

🟢 Approval recommended

No unresolved blocking issues remain, and the encoding and device-state changes are supported by the review evidence.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@mangupta

Copy link
Copy Markdown
Contributor

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

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

@iassiour
iassiour force-pushed the users/iassiour/clr-lds-carveout-no-preference branch from 2ca3824 to bec0b14 Compare October 28, 2026 18:17
@iassiour iassiour changed the title fix(clr): honour no-preference LDS carveout and report device cache c… fix(clr): honor no-preference LDS carveout; fix hipDeviceGetCacheConfig Oct 29, 2026
@mangupta

Copy link
Copy Markdown
Contributor

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

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

@iassiour
iassiour force-pushed the users/iassiour/clr-lds-carveout-no-preference branch from bec0b14 to d433a31 Compare October 29, 2026 21:44
@mangupta

Copy link
Copy Markdown
Contributor

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

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

iassiour and others added 3 commits October 30, 2026 12:55
…onfig

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
@iassiour
iassiour force-pushed the users/iassiour/clr-lds-carveout-no-preference branch from d433a31 to 0c1524c Compare October 30, 2026 12:55
@iassiour
iassiour merged commit 14756c4 into develop Nov 1, 2026
399 of 413 checks passed
@iassiour
iassiour deleted the users/iassiour/clr-lds-carveout-no-preference branch November 1, 2026 16: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