Repository navigation
[rocjitsu] Fix DS transpose reads ignoring AccVGPR acc bit - #6829
Merged
Merged
Conversation
The four CDNA4 DS transpose read instructions (ds_read_b64_tr_b4, ds_read_b96_tr_b6, ds_read_b64_tr_b8, ds_read_b64_tr_b16) ignored the `acc` bit in the DS encoding, always writing results to VGPRs even when acc=1 indicated AccVGPR destination. The fix adds the same `(inst_.acc ? 256u : 0u)` offset to dst_reg_base that all other DS instructions already use. The Python code generator (_generator.py) is also fixed so that regeneration produces the correct output. Two tests are added (I am happy to drop one of them is requested) - DsTransposeTest.ReadB64TrB16_AccBit: encoding-level test that verifies acc=1 routes the write to AccVGPR and leaves VGPR untouched. - DsTransposeAccTest.AccBitWritesToAccVGPR: end-to-end HIP test using inline asm with ds_read_b64_tr_b16 a[2:3] that confirms VGPR v2 keeps its sentinel value while AccVGPR a2 receives the LDS data. Both tests fail without the fix and pass with it. Background: This bug was found by comparing the emulator's race detection output against llvm-objdump disassembly of an AITER SDPA backward kernel The race detector reported a false positive RAW hazard between ds_read_b64_tr_b16 and buffer_atomic_add_f32 on register v6, but llvm-objdump showed the ds_read destination as a[6:7] (AccVGPR), not v[6:7]. This revealed that the emulator was writing to the wrong register file, causing both incorrect simulation results and spurious race reports.
newling
force-pushed
the
fix-ds-transpose-acc
branch
from
June 5, 2026 18:08
ddc87c3 to
9568ea4
Compare
1 task done
kuhar
pushed a commit
that referenced
this pull request
Aug 18, 2026
…0175) ## Motivation `ds_read_b64_tr_b16` returns incorrect results under the gfx950 (CDNA4) simulator. The instruction performs a 4x16-lane halfword transpose: within each 16-lane group, destination lane `l` halfword `n` comes from source lane `((l & ~0xf) | ((l >> 2) & 3)) + 4 * n`, halfword `l & 3` — the layout already encoded by the DBT reference in `tests/dbt/cdna4_to_cdna3_lds_hip_test.cpp` (`LargeStaticLdsReadB64TrB16`). The pure-simulation path's `transpose_b16()` instead performed a 4x4 transpose within groups of 4 consecutive lanes (lane 0 received source lanes 0,1,2,3 halfword 0 instead of lanes 0,4,8,12), so every gfx950 kernel that loads bf16/f16 MFMA operands with `ds_read_b64_tr_b16` computed on wrong data under the simulator. ## Technical Details - `lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/shared/ds_transpose.h`: `transpose_b16()` now handles the wave64 B64 case (4 halfwords per lane, i.e. `ds_read_b64_tr_b16`) with the 4x16-lane stride-4 layout above. This changes only that path; the B128 path used by gfx1250 `ds_load_tr16_b128` / `global_load_tr16_b128` and RDNA4 `global_load_tr_b128` is left byte-for-byte unchanged. `tr_b4` / `tr_b6` / `tr_b8` are untouched and were not hardware-verified here. - `tests/amdgpu_vm_test.cpp`: new `DsTransposeTest.ReadB64TrB16_LaneLayout` (CPU-only). Fills LDS with a per-lane halfword pattern, dispatches an encoded `ds_read_b64_tr_b16` on the `cdna4` VM fixture, and checks all 64 lanes x 4 halfwords against the same reference formula the DBT test uses. - `TransposeKind` is split into `TR16_B128` (gfx1250/RDNA4 B128 loads, unchanged) and `B64_TR_B16` (CDNA4 `ds_read_b64_tr_b16`), emitted from `InstructionSemantics`; `transpose_response()` dispatches on the kind. ## Issue Tracking No existing GitHub issue. Related: #6829 (AccVGPR `acc` bit handling for the same DS transpose reads). ## Test Plan - `ctest` / `rocjitsu_tests --gtest_filter='DsTransposeTest.*'` for the new CPU-side layout test, before and after the `transpose_b16()` change. - Existing test suite for regressions. ## Test Result - `DsTransposeTest.ReadB64TrB16_LaneLayout`: built and run locally (CPU only, GCC 13, Release): fails on the pre-change `transpose_b16()` with 128 mismatches (64 lanes x 2 dwords; e.g. lane 0 low dword 0x12111200 vs expected 0x12441200) and passes after. - Cross-checked on gfx950 hardware (ROCm 7.2.0) with an equivalent inline-asm `ds_write_b64`/`ds_read_b64_tr_b16` kernel: hardware matches the expected layout; the simulator matched only after this change (all 64 lanes differed before). - Full `ctest` shows no status changes versus `develop` other than the added `DsTransposeTest.ReadB64TrB16_LaneLayout`. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
atgutier
pushed a commit
that referenced
this pull request
Aug 19, 2026
…0175) ## Motivation `ds_read_b64_tr_b16` returns incorrect results under the gfx950 (CDNA4) simulator. The instruction performs a 4x16-lane halfword transpose: within each 16-lane group, destination lane `l` halfword `n` comes from source lane `((l & ~0xf) | ((l >> 2) & 3)) + 4 * n`, halfword `l & 3` — the layout already encoded by the DBT reference in `tests/dbt/cdna4_to_cdna3_lds_hip_test.cpp` (`LargeStaticLdsReadB64TrB16`). The pure-simulation path's `transpose_b16()` instead performed a 4x4 transpose within groups of 4 consecutive lanes (lane 0 received source lanes 0,1,2,3 halfword 0 instead of lanes 0,4,8,12), so every gfx950 kernel that loads bf16/f16 MFMA operands with `ds_read_b64_tr_b16` computed on wrong data under the simulator. ## Technical Details - `lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/shared/ds_transpose.h`: `transpose_b16()` now handles the wave64 B64 case (4 halfwords per lane, i.e. `ds_read_b64_tr_b16`) with the 4x16-lane stride-4 layout above. This changes only that path; the B128 path used by gfx1250 `ds_load_tr16_b128` / `global_load_tr16_b128` and RDNA4 `global_load_tr_b128` is left byte-for-byte unchanged. `tr_b4` / `tr_b6` / `tr_b8` are untouched and were not hardware-verified here. - `tests/amdgpu_vm_test.cpp`: new `DsTransposeTest.ReadB64TrB16_LaneLayout` (CPU-only). Fills LDS with a per-lane halfword pattern, dispatches an encoded `ds_read_b64_tr_b16` on the `cdna4` VM fixture, and checks all 64 lanes x 4 halfwords against the same reference formula the DBT test uses. - `TransposeKind` is split into `TR16_B128` (gfx1250/RDNA4 B128 loads, unchanged) and `B64_TR_B16` (CDNA4 `ds_read_b64_tr_b16`), emitted from `InstructionSemantics`; `transpose_response()` dispatches on the kind. ## Issue Tracking No existing GitHub issue. Related: #6829 (AccVGPR `acc` bit handling for the same DS transpose reads). ## Test Plan - `ctest` / `rocjitsu_tests --gtest_filter='DsTransposeTest.*'` for the new CPU-side layout test, before and after the `transpose_b16()` change. - Existing test suite for regressions. ## Test Result - `DsTransposeTest.ReadB64TrB16_LaneLayout`: built and run locally (CPU only, GCC 13, Release): fails on the pre-change `transpose_b16()` with 128 mismatches (64 lanes x 2 dwords; e.g. lane 0 low dword 0x12111200 vs expected 0x12441200) and passes after. - Cross-checked on gfx950 hardware (ROCm 7.2.0) with an equivalent inline-asm `ds_write_b64`/`ds_read_b64_tr_b16` kernel: hardware matches the expected layout; the simulator matched only after this change (all 64 lanes differed before). - Full `ctest` shows no status changes versus `develop` other than the added `DsTransposeTest.ReadB64TrB16_LaneLayout`. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. (cherry picked from commit 95b6c41)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The four CDNA4 DS transpose read instructions (ds_read_b64_tr_b4, ds_read_b96_tr_b6, ds_read_b64_tr_b8, ds_read_b64_tr_b16) ignored the
accbit in the DS encoding, always writing results to VGPRs even when acc=1 indicated AccVGPR destination.The fix adds the same
(inst_.acc ? 256u : 0u)offset to dst_reg_base that all other DS instructions already use. The Python code generator (_generator.py) is also fixed so that regeneration produces the correct output.Two tests are added (I am happy to drop one of them if requested)
Background: This bug was found by comparing the emulator's race detection output against llvm-objdump disassembly of an AITER SDPA backward kernel The race detector reported a false positive RAW hazard between ds_read_b64_tr_b16 and buffer_atomic_add_f32 on register v6, but llvm-objdump showed the ds_read destination as a[6:7] (AccVGPR), not v[6:7]. This revealed that the emulator was writing to the wrong register file, causing both incorrect simulation results and spurious race reports.