Repository navigation
[rocjitsu] Fix DirectToLDS missing workgroup LDS base offset - #5747
Merged
Merged
Conversation
DirectToLDS buffer loads (buffer_load with lds=1) write data from global memory directly into LDS. The emulator computed the LDS write address as M0 + lane_id * data_size, but omitted the workgroup's LDS base offset. DS instructions (ds_read/ds_write) correctly add wf.lds_base() to every address (addr_calc_scalar.h:73). This mismatch meant DirectToLDS writes from workgroup N>0 landed in workgroup 0's LDS region, while DS reads from the correct region found stale data. The bug only manifests when multiple workgroups share a CU. The emulator config's simd_count controls this: simd_count=1024 (default) packs more workgroups per CU, triggering the bug. simd_count=256 masks it. The root fix is in the codegen generator (_generator.py:3865) which emits the DirectToLDS code for all ISA generations. The generated mubuf.cpp files were manually updated with sed to match (the codegen regeneration script was not available to run). Reproducer (fails without fix, passes with fix): import torch torch.backends.cuda.matmul.allow_tf32 = True torch.manual_seed(42) A = (torch.randint(0, 2, (256, 128), device="cuda", dtype=torch.float32) * 2 - 1) B = A.T.clone() C = torch.mm(A, B) torch.cuda.synchronize() C_ref = torch.mm(A.cpu(), B.cpu()) err = (C.cpu() - C_ref).norm() / C_ref.norm() # Without fix: err ≈ 0.675 # With fix: err = 0.0 Fixes: ROCm#5716 Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
newling
force-pushed
the
fix/direct-to-lds-wg-offset
branch
from
May 4, 2026 20:59
2468a36 to
232dceb
Compare
atgutier
approved these changes
May 5, 2026
1 task done
atgutier
pushed a commit
that referenced
this pull request
Aug 17, 2026
…#10192) ## Motivation `buffer_load_* ... lds` ignores the instruction's immediate `offset` when computing the LDS destination address in the simulator. Per the CDNA3/CDNA4 ISA reference guides, §9.1.9 "Memory Buffer Load to LDS": ``` LDS_ADDR = LDSbase + LDS_offset (M0) + inst_offset + (TIDinWave * 4) MEM_ADDR = Base + mem_offset (soffset) + inst_offset + off_vgpr + stride * (idx_vgpr + TIDinWave) ``` `inst_offset` feeds both addresses (LLVM `IntrinsicsAMDGPU.td`, `int_amdgcn_load_to_lds`, which also covers buffer fat pointers: "imm offset (applied to both input and LDS address)"). Kernels that set M0 once and issue several `buffer_load_dword ... offen offset:0/256/512 lds` to fill consecutive LDS tiles therefore kept overwriting the first tile under the simulator. ## Technical Details - `lib/python/amdisa/codegen/_generator.py`: the MUBUF LDS-load template now sets `d->lds_base = wf.m0() + inst_.offset + wf.lds_base()`. `soffset` and the VGPR offset stay memory-side only, matching the formula above. - Regenerated `generated/{cdna1,cdna2,cdna3,cdna4,rdna1,rdna2}/mubuf_exec.cpp` (documented `python -m amdisa --multi ... --gen-isas` flow + clang-format). With the in-tree ISA XMLs the unmodified generator reproduces every checked-in `mubuf*` file byte-for-byte; the only resulting change is that one line in the 18 `buffer_load_*` LDS branches per file. gfx1250's `*_load_async_to_lds_*` (per-lane VGPR LDS address, separate template) and rdna3_5's `buffer_load_lds_*` stubs (`UnimplementedInst`) are untouched; rdna3/rdna4 generate no MUBUF LDS-load path. - `tests/amdgpu_vm_test.cpp`: `MubufLdsTest.LoadDwordLdsAppliesInstOffset` (CPU-only, cdna1-4): M0 = 0x200, two encoded `buffer_load_dword v1, s[4:7], 0 offen [offset:256] lds` from two distinct source rows, all 64 lanes of both LDS rows checked. ## Issue Tracking No existing issue; related: #5747 (workgroup `lds_base` term on the same path). ## Test Plan - `rocjitsu_tests --gtest_filter='MubufLdsTest.*'` before and after the generator change. - `python -m pytest lib/python/amdisa/tests` and full `ctest` for regressions. ## Test Result - `MubufLdsTest.LoadDwordLdsAppliesInstOffset`: on unmodified `develop` it fails for all four CDNA targets (lane 0: LDS+0x200 reads 0xB0000000 instead of 0xA0000000 because the `offset:256` load overwrote row 0; LDS+0x300 still holds the 0xDEADBEEF fill); passes with this change. - Cross-checked on gfx950 hardware (ROCm 7.2.0) with an equivalent inline-asm kernel: the two rows land at LDS+0 and LDS+256 on hardware; the simulator matches only with this change. - Full `ctest`: no status changes versus `develop` other than the added `MubufLdsTest.LoadDwordLdsAppliesInstOffset`. - `python -m pytest lib/python/amdisa/tests`: 1319 passed, 46 skipped. ## 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
…#10192) ## Motivation `buffer_load_* ... lds` ignores the instruction's immediate `offset` when computing the LDS destination address in the simulator. Per the CDNA3/CDNA4 ISA reference guides, §9.1.9 "Memory Buffer Load to LDS": ``` LDS_ADDR = LDSbase + LDS_offset (M0) + inst_offset + (TIDinWave * 4) MEM_ADDR = Base + mem_offset (soffset) + inst_offset + off_vgpr + stride * (idx_vgpr + TIDinWave) ``` `inst_offset` feeds both addresses (LLVM `IntrinsicsAMDGPU.td`, `int_amdgcn_load_to_lds`, which also covers buffer fat pointers: "imm offset (applied to both input and LDS address)"). Kernels that set M0 once and issue several `buffer_load_dword ... offen offset:0/256/512 lds` to fill consecutive LDS tiles therefore kept overwriting the first tile under the simulator. ## Technical Details - `lib/python/amdisa/codegen/_generator.py`: the MUBUF LDS-load template now sets `d->lds_base = wf.m0() + inst_.offset + wf.lds_base()`. `soffset` and the VGPR offset stay memory-side only, matching the formula above. - Regenerated `generated/{cdna1,cdna2,cdna3,cdna4,rdna1,rdna2}/mubuf_exec.cpp` (documented `python -m amdisa --multi ... --gen-isas` flow + clang-format). With the in-tree ISA XMLs the unmodified generator reproduces every checked-in `mubuf*` file byte-for-byte; the only resulting change is that one line in the 18 `buffer_load_*` LDS branches per file. gfx1250's `*_load_async_to_lds_*` (per-lane VGPR LDS address, separate template) and rdna3_5's `buffer_load_lds_*` stubs (`UnimplementedInst`) are untouched; rdna3/rdna4 generate no MUBUF LDS-load path. - `tests/amdgpu_vm_test.cpp`: `MubufLdsTest.LoadDwordLdsAppliesInstOffset` (CPU-only, cdna1-4): M0 = 0x200, two encoded `buffer_load_dword v1, s[4:7], 0 offen [offset:256] lds` from two distinct source rows, all 64 lanes of both LDS rows checked. ## Issue Tracking No existing issue; related: #5747 (workgroup `lds_base` term on the same path). ## Test Plan - `rocjitsu_tests --gtest_filter='MubufLdsTest.*'` before and after the generator change. - `python -m pytest lib/python/amdisa/tests` and full `ctest` for regressions. ## Test Result - `MubufLdsTest.LoadDwordLdsAppliesInstOffset`: on unmodified `develop` it fails for all four CDNA targets (lane 0: LDS+0x200 reads 0xB0000000 instead of 0xA0000000 because the `offset:256` load overwrote row 0; LDS+0x300 still holds the 0xDEADBEEF fill); passes with this change. - Cross-checked on gfx950 hardware (ROCm 7.2.0) with an equivalent inline-asm kernel: the two rows land at LDS+0 and LDS+256 on hardware; the simulator matches only with this change. - Full `ctest`: no status changes versus `develop` other than the added `MubufLdsTest.LoadDwordLdsAppliesInstOffset`. - `python -m pytest lib/python/amdisa/tests`: 1319 passed, 46 skipped. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. (cherry picked from commit c810647)
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.
Fixes: #5716
DirectToLDS buffer loads (buffer_load with lds=1) write data from global memory directly into LDS. The emulator computed the LDS write address as M0 + lane_id * data_size, but omitted the workgroup's LDS base offset. DS instructions (ds_read/ds_write) correctly add wf.lds_base() to every address (addr_calc_scalar.h:73). This mismatch meant DirectToLDS writes from workgroup N>0 landed in workgroup 0's LDS region, while DS reads from the correct region found stale data.
The bug only manifests when multiple workgroups share a CU. The emulator config's simd_count controls this: simd_count=1024 (default) packs more workgroups per CU, triggering the bug. simd_count=256 masks it.
The fix adds wf.lds_base() to the DirectToLDS base address in all ISA generations (cdna1-4, rdna1-2).
Note: I have not actually regenerated the .cpp files, is this possible on my end yet?
Reproducer described in: #5716 is fixed
Motivation
Technical Details
JIRA ID
Test Plan
Test Result
Submission Checklist