Repository navigation
fix(rocjitsu): D16 Loads Count as Partial Defs - #8344
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
The risk here is that _D16_LOAD_CLASSES advertises buffer D16 coverage, but the generated source operands still do not include the preserved destination for MUBUF/VBUFFER loads. _dst_is_also_source() can return true for buffer_load_d16_u8, yet the emission gates at emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py:1810 and emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py:5215 only accept output operands named vdst or sdst; buffer loads use vdata. The generated emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/rdna4/vbuffer.cpp:867 still has vdata only in dst_operands_, with sources starting at vaddr, so liveness still treats those D16 buffer loads as full defs. If buffer loads are in scope, please include the vdata path and regenerate the affected files; otherwise remove buffer_load/tbuffer_load from the D16 class set so the contract matches the generated behavior.
|
There are also some pytest failures |
|
🎉 All checks passed! This PR is ready for review. |
d32872d to
ebad804
Compare
## Motivation Adds the destination register for SDWA (dst_unused:preserved) and DPP (partial mask) instructions as a source so that InstDefUse and liveness analysis work soundly. Addresses issue #7033. PR #8344 is a followup. ## Test Plan Wrote some tests cases that decode SDWA and DPP instructions that do and don't keep prior register values and checked if uses were stored properly. ## Test Result Passed new tests and all previous tests. --------- Co-authored-by: Allyson Cauble-Chantrenne <acaublec@amd.com>
|
@theSK2005 could you rebase please? |
|
Just to check I understand the description
My understanding is that before this PR:
and then after this PR, it is either option A or B below? This PR caught my attention because I've encountered a semantically problem in a parallel place in rocjitsu #8628. |
This PR doesn't change the operand sizes, so it's still seen as a 32-bit write and 0-bit read. What this PR changes is that the dst register is counted as an implicit use so that they're surfaced to liveness analysis. If you look at the regenerated ISA on this PR, it's all implicit use functions. |
ebad804 to
fac570e
Compare
accauble
left a comment
accauble
left a comment
There was a problem hiding this comment.
I have one minor concern but otherwise LGTM.
…artial loads are tracked properly in liveness
…nd added vdata parameter to cover buffer loads
fac570e to
42aa6b0
Compare
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
Original anchor: emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py:2784
Can we make the FORMAT D16 cases consistent before this lands? The current predicate only admits num_elems == 1, and the generated output shows two holes in that model: tbuffer_load_format_d16_x gets an implicit_uses() override, but buffer_load_format_d16_x / RDNA4 buffer_load_d16_format_x do not, while odd forms like tbuffer_load_format_d16_xyz write 3 halfwords into a 64-bit vdata and leave the upper half of the second VGPR preserved. This means liveness can still treat real FORMAT D16 loads as full defs. I think we should either model those preserved reads (for example, emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/cdna3/mubuf.cpp:246, emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/rdna4/vbuffer.cpp:228, and emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/cdna3/mtbuf.cpp:443) and add decode tests for *_format_d16_x and *_format_d16_xyz, or explicitly document/test why these formats are out of scope.
At emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/gfx1250/vflat.cpp:316, the D16 preserved-destination read is emitted only as implicit_uses(). On gfx1250, InstDefUse resolves VGPR implicit reads with implicit_use_operands() and then drops RegClass::VGPR from the flat implicit_uses() result, so this vdst dependency is lost for banked VGPR liveness. We should emit the operand-backed implicit_use_operands() twin for D16 load destinations on VGPR-MSB profiles, and add a gfx1250 decode/liveness test for this path.
|
Agent review at https://github.com/newling/agent-public-reviews/blob/main/rocm-systems/review_pr8344.md highlights 2 actionable items
Related to earlier discussion (from 2 weeks ago... apologies for my glacial response time on this) longer term I wonder if we should be modeling this at the byte level rather than the dword (4-byte) level. Depending on what tools (consan?) use this, it might be necessary. For future consideration |
|
Agent review says there is just 1 outstanding issue from a previous review (not sure whose), please take a look and either address it or say if it's not valid: https://github.com/newling/agent-public-reviews/blob/main/rocm-systems/review_pr8344_followup.md#1-preserve-d16-destination-reads-in-gfx1250-bank-aware-liveness |
yep, I'm working on fixing that atm |
Thanks. LGTM now, no more comments/questions on my side. I'll let one of the main reviewers listed make any final reviews and stamp when ready |
atgutier
left a comment
atgutier
left a comment
There was a problem hiding this comment.
[MAJOR] /tmp/pr8344/emulation/rocjitsu/lib/python/amdisa/semantics.py:2090
— D16 FORMAT classification activates incorrect load/store execution
Reasoning: Adding _BUFFER_FORMAT_MAP to MUBUF derivation changes
previously unimplemented BUFFER FORMAT operations into executable memory
operations. The execution pipeline does not implement the packed layout—
two 16-bit components per VGPR—that the new semantics describe.
Evidence:
-
/tmp/pr8344/emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/
memory_pipeline.cpp:137 uses floor division for the destination VGPR
count despite its “rounded up” comment. -
Lines 169–194 copy only two bytes at each four-byte VGPR offset and
preserve the high half of every visited register. -
Consequently, D16 XY writes only X and preserves the old high half
instead of writing Y. D16 XYZ calculates one destination VGPR for six
bytes and never writes vdata+1, despite its generated two-register
definition and preserve-read of the last register at /tmp/pr8344/
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/rdna4/
vbuffer.cpp:442. -
Stores are also wrong: /tmp/pr8344/emulation/rocjitsu/lib/rocjitsu/src/
rocjitsu/isa/arch/amdgpu/rdna4/vbuffer.cpp:558 reads the low half of the
same VGPR for both XY components. XYZ and XYZW similarly repeat low
halves rather than extracting packed components. -
The added tests validate semantic metadata and def/use sets, but never
execute multi-component D16 loads or stores.
Suggested fix: Either keep BUFFER FORMAT execution unclassified while
deriving partial-definition metadata separately, or implement packed-D16
execution comprehensively: ceil the VGPR count, pack/unpack two components
per dword, and preserve only the unused half of the final register for odd
component counts. Cover normal and OOB completion, plus XY/XYZ/XYZW load
and store execution tests using distinct halfword values.
Confidence: High.
[MAJOR] /tmp/pr8344/emulation/rocjitsu/lib/python/amdisa/15:42:07 [43/761]
— RDNA4 typed D16 loads remain missing from the fix
Reasoning: RDNA4 combines untyped and typed buffer instructions under
ENC_VBUFFER, but VBUFFER is routed only to derive_mubuf, which recognizes
BUFFER, not TBUFFER_. Therefore typed D16 X and XYZ loads receive no
preserved-destination use, leaving DBT liveness unsound.
Evidence:
-
_derive_mubuf accepts only BUFFER prefixes at /tmp/pr8344/emulation/
rocjitsu/lib/python/amdisa/semantics.py:2087. -
An exact-head semantic probe classified both TBUFFER_LOAD_D16_FORMAT_X
and ...XYZ under ENC_VBUFFER as nop. -
RDNA4 X at /tmp/pr8344/emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/
arch/amdgpu/rdna4/vbuffer.cpp:3394 defines vdata but has no
implicit_uses() override. -
XYZ at /tmp/pr8344/emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/
arch/amdgpu/rdna4/vbuffer.cpp:3442 defines vdata:vdata+1 but does not
preserve-read the unused half of the final register. -
These are the unified-encoding equivalents of the pre-RDNA4 TBUFFER
instructions that this PR does handle.
Suggested fix: Give VBUFFER derivation support for both BUFFER and TBUFFER
families, or detect partial definitions independently of execution
classification. Regenerate RDNA4 and add real-decode def/use tests for
typed D16 X, XY, XYZ, and XYZW.
Confidence: High.
[MINOR] /tmp/pr8344/emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/
amdgpu/cdna1/mubuf.cpp:1470 — LDS-destination loads falsely keep vdata
live
Reasoning: On older MUBUF encodings, inst_.lds redirects the loaded value
exclusively to LDS. The new hook nevertheless unconditionally reports the
encoded vdata VGPR as a preserved-destination read.
Evidence:
-
The implicit use is unconditional.
-
The LDS path at /tmp/pr8344/emulation/rocjitsu/lib/rocjitsu/src/
rocjitsu/isa/arch/amdgpu/cdna1/mubuf.cpp:1477 sets lds_dst, does not
establish a VGPR destination, and returns. -
LDS completion is selected at /tmp/pr8344/emulation/rocjitsu/lib/
rocjitsu/src/rocjitsu/vm/amdgpu/memory_pipeline.cpp:126. -
This cannot under-approximate liveness, but it creates a false live
range and can unnecessarily exclude a DBT scratch register.
Suggested fix: For encodings with an LDS field, emit the D16 implicit use
only when !inst_.lds. Add def/use tests for the same opcode with LDS clear
and set.
Confidence: High.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
The risk here is that D16 FORMAT loads/stores decode with GPUMEM operands but are not marked as memory ops. At emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py:7116, _MEM_CLASSES includes buffer_load and buffer_store but not the new buffer_load_format_d16 or buffer_store_format_d16 classes; the generated emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/rdna4/vbuffer.cpp:348 constructors set gpumem but never flags_ |= MEMORY_OP. We should either add both metadata-only classes to the memory flag contract or leave an explicit test/comment if Instruction::is_memory_op() is meant to return false for these memory instructions.
It would help to tighten this D16 FORMAT liveness test block in a couple of targeted spots. emulation/rocjitsu/tests/analysis/liveness_test.cpp:4224 only checks !uses.contains({VGPR, 5, 2}); since RegisterSet::contains requires every lane in the range, a regression that reads only v5 or only v6 would still pass, so please assert both single registers are absent. Also, the new VBUFFER typed path is covered only by tbuffer_load_d16_format_x at emulation/rocjitsu/tests/analysis/liveness_test.cpp:4280; adding one tbuffer_load_d16_format_xyz decode would lock in the odd-count typed case where only the last VGPR is preserved.
…-partial-load # Conflicts: # emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py # emulation/rocjitsu/tests/analysis/liveness_test.cpp
Motivation
D16 memory loads are 16 bit writes into VGPRs. These operations currently are seen as full defs by liveness analysis since VDST is only a dst and not a src and has a 32 bit operand size so PR #7979 won't cover it.
Technical Details
Added check in _generator.py's _dst_is_also_source to cover d16 load operations and have codegen write VDST as both a dst and a src.
Fixed existing semantics.py bug that didn't use _BUFFER_FORMAT_MAP for derive_mubuf which left many instructions unimplemented and not properly flagged.
Test Plan
Test Result
All tests pass
Submission Checklist