Skip to content

[rocjitsu] Add CDNA3/CDNA4 global_load_lds_* execution support - #14867

Open
RattataKing wants to merge 6 commits into
developfrom
users/RattataKing/add-lds-inst
Open

RattataKing wants to merge 6 commits into
developfrom
users/RattataKing/add-lds-inst

Conversation

@RattataKing

@RattataKing RattataKing commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Adds the five single-dword GLOBAL LDS loads on CDNA3 and CDNA4 in the generator:

  • global_load_lds_ubyte
  • global_load_lds_sbyte
  • global_load_lds_ushort
  • global_load_lds_sshort
  • global_load_lds_dword

Add tests:

  • GlobalLoadsToLdsPreserveInactiveLanesAndVgprs

Issue ID: #13025

@therock-pr-bot

therock-pr-bot Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

therock-pr-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update the test documentation to match the new execution support:

  • emulation/rocjitsu/tests/flat_lds_transfer_test.cpp:372: replace the claim that GLOBAL LDS execution remains unimplemented and belongs to PR #13218. This case checks decoding; execution is now covered by GlobalLoadsToLdsPreserveInactiveLanesAndVgprs and the CDNA4 wide-load tests.
  • Optional: emulation/rocjitsu/tests/flat_lds_transfer_test.cpp:271: make the skip reason explicit, for example “This decoder boundary is specific to CDNA3.” That explains why other architectures skip, whereas “Checks the CDNA3 decoder boundary” only describes the test.

@RattataKing
RattataKing force-pushed the users/RattataKing/add-lds-inst branch from 787fa02 to f8c8c7c Compare November 4, 2026 23:57
@RattataKing
RattataKing requested a review from kuhar November 4, 2026 23:57

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one note to consider

@RattataKing
RattataKing force-pushed the users/RattataKing/add-lds-inst branch from f8c8c7c to d5cf8a4 Compare November 6, 2026 01:22

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we trim the duplicate X3/X4 code now that #14842 is in the base? This PR adds execution for the five single-dword destination forms. The generated CDNA4 X3/X4 bodies already match the base.

  • In emulation/rocjitsu/lib/python/amdisa/semantics.py:2177–2178, we can drop the two wide suffixes. Line 2152 handles them first, so editing the later entries would leave both opcodes unchanged.
  • In emulation/rocjitsu/tests/flat_lds_transfer_test.cpp:77, the CDNA3 rejection test can reuse global_wide_lds_encoding from line 71. The wide tests at lines 475 and 516 already cover stride, X3 padding, inactive lanes and whole-payload bounds. Folding the exact-fit and one-byte-short cases from line 254 into the existing bounds test would preserve that coverage with one suite to maintain.

This cleanup can follow separately.

LGTM

@RattataKing
RattataKing force-pushed the users/RattataKing/add-lds-inst branch from d5cf8a4 to 4733bb1 Compare November 6, 2026 05:14
@peymanr

peymanr commented Nov 6, 2026

Copy link
Copy Markdown

PR Pundit review

(Testing PR Pundit Code Review agent. Not all comments may be relevant. This is a one-time automated pass: please address what is useful, no need to request a re-review. Please reply to each point, whether it applies or not: we harvest these replies, and both kinds are enormously helpful for tuning the agent. Reach out to @peymanr for suggestions.)

# Finding Where
1 🟠 Narrow GLOBAL LDS regression on subword/sign-extend/M0 would go uncaught: this PR's test uses only aligned addresses and M0=64 emulation/rocjitsu/tests/flat_lds_transfer_test.cpp:157

1. 🟠 Should fix: Narrow GLOBAL LDS regression on subword/sign-extend/M0 would go uncaught: this PR's test uses only aligned addresses and M0=64

emulation/rocjitsu/tests/flat_lds_transfer_test.cpp:157

The new GlobalLoadsToLdsPreserveInactiveLanesAndVgprs test only uses dword-aligned source addresses, payload bytes/shorts with the top bit set (0x81, 0x8081), offsets -4/0/4 and a clean M0=64. So subword addr&3 extraction, zero- vs sign-extension of positive values, and M0 masking are not exercised on the GLOBAL path specifically. The generated GlobalLoadLds* execute bodies are identical to the ScratchLoadLds* ones, which ScratchWidthsAndAddressModesPreserveInactiveLanesAndVgprs covers with per-lane subwords and M0=0xfffc0043, so the risk is low. Adding a subword offset, a positive payload and an unaligned M0 to the GLOBAL test would make it independent of the scratch coverage.

Who is affected: Only test coverage of the emulator's CDNA3/CDNA4 narrow GLOBAL_LOAD_LDS_* path; no runtime behaviour is affected.

Checked against the code by an independent pass.

Related past reviews: #6886 on emulation/rocjitsu/lib/python/amdisa/tests/test_sema_derive.py, #6886 on emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py, #10199 on emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/cdna5/addr_calc.cpp, #10847 on emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/cdna5/addr_calc.cpp

Shared from a pilot of PR Pundit, an agentic code reviewer that builds and tests the change and reads the project's past reviews. Reviewed at 4733bb13; findings checked and posted by @peymanr; points already raised on this PR are left out.

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we update the PR description to say this adds the five single-dword GLOBAL LDS loads on CDNA3 and CDNA4? The CDNA4 X3/X4 instructions landed in #14842. This revision extends their existing tests with cases such as exact-fit and one-byte-short LDS allocations, so the reference to Cdna4WideGlobalLoadsUseSixteenByteLdsLaneStride can go too.

LGTM

This branch has not been deployed

No deployments
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.

3 participants