Repository navigation
[rocjitsu] Use the ISA scratch layout for RDNA3/3.5/4 and fix CDNA private accesses - #13996
yashjha123 wants to merge 11 commits into
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
There was a problem hiding this comment.
🟡 Changes recommended
Private 64-bit FLAT atomics bypass swizzle-aware transfers, causing incorrect results and possible neighboring-lane corruption.
1 open issue
What changed in this PR
This PR extends CDNA’s dword-interleaved scratch layout to RDNA3, RDNA3.5, and RDNA4, aligning emulator memory placement with debugger expectations and improving scratch-access locality.
Changes:
- Apply interleaving to SCRATCH instructions and FLAT private-aperture accesses.
- Align FLAT wait planning and remove the unused linear layout.
- Update regression expectations and add wave-size and mixed private/global address checks.
| File | Description |
|---|---|
emulation/rocjitsu/tests/shared_infra_test.cpp |
Checks interleaved addresses, wave sizes, and mixed FLAT lanes. |
emulation/rocjitsu/tests/instruction_execution_harness_test.cpp |
Updates scratch backing expectations in execution tests. |
emulation/rocjitsu/tests/execution_plugin_race_test.cpp |
Updates private-memory placement in race-detector tests. |
emulation/rocjitsu/tests/execution_plugin_memory_test.cpp |
Updates scratch backing for routed-memory counter tests. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/memory_wait_scoreboard.cpp |
Selects interleaved RDNA private addresses for wait planning. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/shared/flat_address.h |
Removes the unused linear private layout. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/rdna4/addr_calc.cpp |
Enables interleaved RDNA4 scratch and private FLAT addressing. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/rdna3/addr_calc.cpp |
Enables interleaved RDNA3 scratch and private FLAT addressing. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/rdna3_5/addr_calc.cpp |
Enables interleaved RDNA3.5 scratch and private FLAT addressing. |
Review effort: Balanced
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
Can we give the dword-interleave calculation one shared helper and use it for FLAT translation and the dedicated SCRATCH paths? Under emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/, the new copies at rdna3/addr_calc.cpp:107, rdna3_5/addr_calc.cpp:106, and rdna4/addr_calc.cpp:198 repeat the mapping in shared/flat_address.h:41 and the CDNA paths.
Keeping the formula and its dword-size constant together would make future layout changes apply consistently to execution and wait planning. It would also give the stride calculations one definition and provide one place to document why the layout must match the ISA and debugger. Architecture-specific operand decoding and signedness can stay in the callers.
599c074 to
0c70fdc
Compare
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
I think we should update the PR title and description to include the CDNA5 behavior changes now in this head. d8688240617 faults 8-byte private FLAT atomics on gfx1250, and 0c70fdcdd2b sign-extends its SCRATCH register offsets before VGPR scaling. These are observable changes outside the RDNA-only title.
Please also describe the new private-atomic fault behavior on RDNA3/3.5/4 and identify which revision the reported test results cover, so readers can distinguish the original layout measurements from validation of these later fixes.
yashjha123
changed the title
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
I think we should narrow the threefry explanation in the PR description to the batching path actually being skipped. In emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/l1_vector_cache.cpp:408, cached_private_ram_mtype gates the inner cross-lane dword batch; store_swizzled itself already runs for scratch when addr_stride != 0 (line 484). Also, legacy_address_space.h:641–643 and :710 require DriverSealedRam ownership in both the policy hint and the eventual batch writer, so changing the hint alone does not establish a working fix. Please distinguish the observed regression from the proposed follow-up and avoid presenting an eligibility-only change as sufficient without validation.
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
We should update the remaining SCRATCH comments to describe the dword-interleaved address mapping. emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/shared/addr_calc_flat.h:39 still documents scratch_base + VGPR + saddr + offset, and emulation/rocjitsu/tests/shared_infra_test.cpp:5952 opens with the same linear-layout claim. Both now use or check the interleaved layout, so these comments give readers the wrong address contract. Please describe operand addition as forming the private byte offset, followed by the lane interleave; the test already explains the correct formula beside its assertions.
Fix two critical issues when performing floating-point compare-swap. These were found when working on a related PR (#13996). Tested against W7900 (gfx1100), RX 6800 XT (gfx1030), and RX 5700 XT (gfx1010) captures. (1) The memory swap was triggered incorrectly, as the floating comparison was confused with an integer comparison. The wrong fallback to the integer path was due to a drift in the implementations (the execution paths now share operand classification and an FP operation mapping). (2) The overwritten value didn't match the HW behaviour. The written values don't flush, but the comparison still flushes them, apparently. `fp_mode::atomic_scalar` uses the flushed values only to decide equality, then returns the original replacement or memory bits. <details> <summary>More details for agent's context</summary> ### What changes | Case | develop | This PR | Measured | |---|---|---|---| | Translated backend, any `FCMPSWAP` (memory 1.0, compare 1.0, replacement 2.0) | memory stays `3f800000`, reports success | memory becomes `40000000` | gfx1100 | | Flushed denormals, match with a denormal replacement (memory 1.0, compare 1.0, replacement `0x1`) | stores `0` | stores `0x1` | gfx1100 global and LDS; gfx1030, gfx1010 LDS | | Flushed denormals, mismatch on denormal memory (memory `0x1`, compare 3.0) | writes `0` | memory untouched | same | | Flushed denormals, mismatch on negative denormal memory (memory `0x80000001`, compare 1.0) | writes `0x80000000` | memory untouched | same | The 32-bit patterns are shown; the 64-bit LDS forms behave the same way with `0x0000000000000001` and `0x8000000000000001`. The returned pre-op value was already correct. Unchanged and confirmed on hardware: −0 equals +0, NaNs never match (including identical quiet and signaling payloads), flushed denormals compare equal to ±0 and to each other, preserved denormals compare by value, and a signaling-NaN replacement is stored without quieting. ### Hardware evidence - `global_atomic_cmpswap_f32` and `ds_cmpstore_rtn_f32/f64` on a W7900 (gfx1100); `ds_cmpst_rtn_f32/f64` on an RX 6800 XT (gfx1030) and an RX 5700 XT (gfx1010). - Inline assembly, so the compiler cannot lower to an integer compare-swap. 18–26 cases per instruction, each run with MODE.FP_DENORM preserving and flushing denormals. Every capture was rerun with identical output, and the gfx1010/gfx1030 LDS results are identical to gfx1100's. - The same probe binaries under rocjitsu's gfx1100 config reproduce the W7900 output byte for byte with this PR; before it, the flushed-denormal rows above differed. ### Changes - `atomic_op.h`: `atomic_uses_two_sources()` adds `FCMPSWAP` and `WRAP` to the translated handler's set. `memory_pipeline.cpp` uses one optional FP operation mapping for both classification and dispatch across the cached, translated and LDS paths. Unsupported evaluator inputs now fail explicitly instead of returning the old value; invalid `ScalarAtomicOp` values are also rejected instead of falling through to max. - `fp_mode.h`: `atomic_scalar`'s `FCMPSWAP` case returns `source_bits`/`old_bits` instead of their flushed copies. ### Tests - `GpuVmPipeline.FloatCompareSwapMatchesAcrossBackends`: synthetic 32- and 64-bit compare-swap requests through both backends, denormals preserved and flushed. The CDNA5 fixture has no decoded `FCMPSWAP` instruction; this checks shared pipeline behaviour. Lanes cover a match, a mismatch, −0 against +0, NaN, a denormal against +0, a denormal mismatch and a denormal replacement; checks memory and the returned value. - `FpModePolicyTest.FloatCompareSwapFlushesOnlyTheComparison`: the measured cases as a table, both widths and both MODE settings. - `AtomicPolicyExecutionTest.DsFloatCompareStoreEqualityAndDenormals`: three expectations corrected. They asserted the flushed write introduced with #11489. RDNA3/3.5 ISA section 13.2 and CDNA2 section 9.5.2 / CDNA3/4 section 9.2.2 specify flushing the CompareStore result, but the captures contradict that rule on the measured paths. Code and tests now state this discrepancy and identify unmeasured targets. - `AtomicPolicyExecutionTest.MemoryFloatCompareStore`: three rows added for a matching denormal replacement and positive and negative denormal mismatches, covering the changed stored bits through the decoded GLOBAL and BUFFER paths on RDNA1–3.5. Its existing flush-mode row passes under either policy; each new row fails on all four architectures and both paths when the selection is reverted to the flushed values. - `AtomicSemanticsAndDecode.IntegerEvaluatorRejectsUnsupportedOperations` and `FpModePolicyTest.ScalarAtomicRejectsUnsupportedOperations` cover both widths and fail with the original evaluator fallbacks. - Each original fix checked by reverting only its library change: the pipeline test fails only on the translated backend for the first, and only in flush mode (both widths, both backends) for the second; the policy test fails exactly the three flush-mode rows per width. ### Results (clang Release) - Before the review fixes, `ctest`: 8,862 of 8,876 passed. The 14 failures (3 `HsaTest`, missing `hsa_amd_queue_create` on the host runtime; 11 `Gfx1250ExecutionTest.TensorDma*` segfaults) also fail on unmodified develop. - At `391d14bb060` (rebased onto `a556b0be0bb`), 521 focused tests pass, 85 are skipped, and none fail. Release Clang 20 linking needed a build-only explicit instantiation for the existing missing `ComputeUnitCore::step_impl<true>` symbol; the workaround is not part of this PR. The full suite and hardware captures have not been rerun for these review fixes. - At `05d78d76078`, the focused atomic, compare-swap and pipeline tests pass (395 passed, 85 skipped), as does `rocjitsu_fp_mode_tests` (35,089 passed). ### Still open - **Unmeasured paths:** CDNA1–4 LDS, RDNA1/2 cache atomics (32/64-bit FLAT/global and BUFFER forms), RDNA3.5 LDS/cache, and gfx1100 FLAT/BUFFER F32 inherit the shared policy without equivalent captures. The gfx1010/gfx1030 captures cover LDS only. In this tree CDNA1–4 reach `FCMPSWAP` only through LDS; RDNA1/2 implement the 64-bit cache `fcmpswap_x2` forms. CDNA5/RDNA4 expose no `FCMPSWAP` instructions. - **Unmeasured MODE settings:** every capture used FP_DENORM 0 or 3, endpoints that cannot separate input-controlled from output-controlled comparison flushing. A capture at FP_DENORM 1 or 2 with denormal memory compared against +0 and a normal replacement would, as this policy swaps only in mode 2. - **`WRAP`:** currently only LDS `ds_wrap` on CDNA1–4 and RDNA1–3.5 uses it, so adding it to the translated handler's two-source set has no visible effect today. </details>
Give the dword-interleaved private layout one definition in shared/flat_address.h: scratch_lane_address() maps a lane's private byte offset to its backing address, and scratch_unit_stride() gives the distance between a lane's consecutive dwords. Use them in the four existing copies of the formula and its stride: dedicated SCRATCH and private-aperture FLAT on the shared CDNA1-4 and RDNA1/2 path, the same two paths on CDNA5, and the Interleaved branch of translate_flat_address. The documentation of why the layout must match the ISA and rocm-dbgapi moves next to the helper. No behavior changes. RDNA3/3.5/4 still use FlatPrivateLayout::Linear.
CDNA5 section 10.2 supports 4-byte FLAT atomics into scratch but returns MEMVIOL for 8-byte ones. The emulator instead ran them through both atomic handlers as eight contiguous bytes. Under the dword interleave a lane's high half is a wave stride away, so the atomic modified the neighbouring lane's private dword even when that lane was inactive. Reject the whole instruction in GlobalMemPipeline::initiate_access, next to the DDS rejection, before any global, LDS or scratch side effect. Only private-backed lanes trigger it, so BUFFER swizzling, 4-byte private atomics and 8-byte global atomics are unchanged. The regression covers swap, compare-swap and add. A global lane issues before the offending private lane, and an inactive neighbour holds a canary. Controls cover a 4-byte private swap and an 8-byte swap whose private lane is disabled.
RDNA scratch addressed each lane's private space contiguously (scratch_base + lane * lane_size + offset). The RDNA3 and RDNA3.5 ISA guides (11.2) define scratch as dword-swizzled with index_stride equal to the wave size: Addr = SCRATCH_BASE + (offset / 4) * 4 * index_stride + offset % 4 + TID * 4 The RDNA4 guide uses the same SWIZZLE(offset, ThreadID) wording, and rocm-dbgapi reads gfx10/11/12 private memory with this 4-byte interleave. CDNA already uses this layout. RDNA's per-lane layout came from #7646, which copied the CDNA convention of the time, and #13019 named it FlatPrivateLayout::Linear to preserve that behavior. Map SCRATCH instructions and private-aperture FLAT lanes through scratch_lane_address(), and mark them as interleaved private lanes so multi-dword accesses split per dword. FLAT wait planning uses the same layout. A spill where every lane uses the same offset now touches one contiguous line instead of one line per lane. FlatPrivateLayout::Linear has no remaining users. Remove it together with FlatAddress::swizzled, which every remaining translation set with private_address; the wait scoreboard now reads private_address. Interleaving moves a lane's high dword a wave stride away, so 8-byte FLAT atomics on private lanes would modify the neighbouring lane's dword. RDNA3/3.5/4 section 11.1 specifies MEMVIOL for them; extend the existing CDNA5 rejection to these targets. Run the layout test on RDNA3 and RDNA3.5 for wave32 and wave64, check mixed private/global FLAT lanes on RDNA3, RDNA3.5 and RDNA4, and update the expectations that encoded RDNA4's per-lane backing. Run the decoded FLAT wait-plan cross-check with b128 loads and returning 32-bit atomics too, so private lanes that split per dword or take the atomic handlers keep the routing execution uses.
RDNA4 section 11.2 and CDNA5 section 10.2 define both SCRATCH register offsets as signed 32-bit values, and CDNA5 scales the VGPR offset after sign extension. Both targets zero-extended the registers, so VGPR=-64 with SGPR=0x100 produced a displacement of 0x20_0000_1800 instead of 0x1800. CDNA5 also scaled the VGPR in 32 bits, wrapping large offsets. Sign-extend each register before the wide sum and leave the layout to scratch_lane_address(). RDNA3 and RDNA3.5 specify unsigned register offsets and keep that behavior. Add negative-register cases for RDNA4 and CDNA5, including CDNA5's scaled VGPR, and pin RDNA3/3.5's unsigned offsets.
120fc81 to
ad3e990
Compare
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
I'm seeing some regressions on my local benchmark suite:
We should understand these before merging.
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
It would help to credit #14352 for signed RDNA4/CDNA5 SCRATCH offsets in the PR description and reword 1681f106bf0 accordingly. The base already sign-extends both registers, including the CDNA5 VGPR before scaling.
The base also has a signed-offset test matrix for both targets, Gfx12AddrCalcTest.ScratchSignedOffsetsAreWidenedBeforeScalingAndAddressing. If you want to trim the new fixtures at emulation/rocjitsu/tests/shared_infra_test.cpp:6643 and :6904, one row with a negative VGPR on one lane and a positive VGPR on the other would preserve their distinct per-lane check.
Can we check whether the +1.61% for gfx942 Gluon at 24 CPUs in the benchmark review exceeds the run-to-run variation between matched base and head builds? gfx942 and gfx950 already split multi-dword scratch transfers on the base, so the new store splitting seen in gfx1201 threefry cannot explain those CDNA rows. If the difference repeats, a profile of that one case would help us decide whether to land this change.
Switch RDNA3, RDNA3.5, and RDNA4 to the ISA-specified swizzled scratch layout, reusing CDNA’s implementation (#9844). Also fix signed SCRATCH offsets on RDNA4/CDNA5 and reject 64-bit private FLAT atomics on RDNA3/3.5/4 and CDNA1–5.
Historical measurements on
a2082765e0eplus the original layout change, before the current rebase and correctness fixes, showed a 1.64× geomean speedup across eight non-nightly workloads: seven attention cases improve 1.59–2.10×, whilethreefry4x32takes 40% longer.Cause and fix
The linear layout groups scratch data by lane, scattering wave accesses across cache lines and pages. The swizzled layout interleaves dwords across lanes, improving locality.
The profiling counters show 2.34x more per-dword cache calls, but 94% fewer L2 line fetches and 96% fewer page-policy lookup misses.
The shared mapping now covers SCRATCH instructions, FLAT private-aperture accesses, and FLAT wait planning. Multi-dword accesses split per dword, matching CDNA5. The unused
FlatPrivateLayout::Linearis removed.Table
Historical extended corpus (gfx1201; not remeasured at current head)
Known regression (Agent's Speculation)
threefry4x32increases from 2.20 s to 3.07 s. Its only scratch instruction,scratch_store_b128, becomes four dword writes per lane. Scratch reachesstore_swizzled, but its inner cross-lane dword batch is skipped. Enabling that batch is a proposed follow-up; both backing-ownership checks and correctness/performance need validation.Manual's Expected Address Space (the change)
For lane
laneand byte offsetoffset:scratch_base + lane * lane_size + offsetscratch_base + (offset / 4) * wave_size * 4 + lane * 4 + offset % 4Full details for agents and humans
The bug
On emulated RDNA3, RDNA3.5 and RDNA4, kernels that spill registers run much slower than on other architectures. Profiling about 170 workloads under rocjitsu on emulated gfx1201, gfx950 and gfx1250 showed scratch traffic dominating emulator time in spill-heavy kernels on gfx1201. gfx1250 did not show this pattern. GPT-OSS attention is a representative case. On gfx1201 it spills about 370 VGPRs per lane, and scratch instructions are about 6% of executed wave instructions.
The scratch layout RDNA used also differs from the layout in the ISA. Ordinary scratch load/store results are unaffected by the layout change, because those paths agreed on the same layout. However, rocm-dbgapi reads private memory using the ISA layout, so ROCgdb attached to an emulated RDNA GPU would read private variables from addresses the emulator never wrote.
The cause
RDNA gave each lane its own contiguous block of scratch:
When every lane in a wave spills to the same offset, which is the usual case, consecutive lanes' dwords are
lane_sizebytes apart. One scratch instruction therefore touches an average of 33.5 cache lines across 12.3 pages. The emulator pays for a cache-line fill and a page-policy lookup on nearly every one, so on GPT-OSS attention its emulated L1 read hit rate is 0.7%.The RDNA3 and RDNA3.5 ISA guides (section 11.2) define scratch as dword-swizzled across the wave, with the index stride equal to the wave size. The RDNA4 guide uses the same
SWIZZLE(offset, ThreadID)wording but gives no explicit formula. rocm-dbgapi reads gfx10–12 private memory with this 4-byte interleave, and CDNA already uses it (#9844).RDNA's per-lane layout was inherited, not chosen:
FlatPrivateLayout::Linearso that its behaviour would not change.The fix
lane, byteoffsetscratch_base + lane * lane_size + offsetscratch_base + (offset / 4) * wave_size * 4 + lane * 4 + offset % 4The new layout applies to SCRATCH instructions, to FLAT accesses that resolve to the private aperture, and to FLAT wait planning, matching CDNA. Multi-dword accesses such as
scratch_store_b128are split into one access per dword, as on CDNA5. In a FLAT wave that mixes private and global lanes, only the private lanes are swizzled.FlatPrivateLayout::Linearhas no remaining users, so it is removed.Buffer (MUBUF) scratch still follows the descriptor's swizzle bits, and the scratch allocation size is unchanged. For ordinary scratch loads and stores, only the location of scratch bytes changes, and that location is what the debugger and the emulator's caches observe.
The address formula and private-transfer setup are shared across SCRATCH and FLAT paths. RDNA4 and CDNA5 SCRATCH register offsets are sign-extended before addition, and before VGPR scaling on CDNA5. Eight-byte private FLAT atomics fault before any lane changes memory: RDNA3/3.5/4 ISA section 11.1.1 and CDNA5 section 10.1.1 specify MEMVIOL; CDNA1–4 are rejected with an unsupported-operation diagnostic because the existing handlers corrupt neighbouring lanes, without claiming a hardware fault policy. Four-byte private atomics and eight-byte global atomics remain supported; BUFFER behaviour is unchanged.
Verification
Performance. Emulated gfx1201 on an idle Ryzen 9 9950X, 16 threads. Each number is the median of 2–3 runs of the timed kernel region, with compile and setup excluded, and all outputs were validated. The baseline was
developata2082765e0e, and the test build was the same commit plus this change, measured before the rebase onto272f1889c8c.Why it is faster. Temporary emulator counters on GPT-OSS attention (window 128, seq 1024) confirm the mechanism. Because multi-dword accesses are now split, the emulator makes more per-dword cache calls, not fewer. The speedup comes from those calls hitting, since each spill now stays on one page and a few cache lines.
Tests.
Rdna4MemoryExecutionHarness.MixedFlatGlobalScratchAndLdsPreserveCanaries, which arrived fromdevelopwith the old expectation.Rdna3ScratchUsesDwordInterleavedLayoutand its RDNA3.5 counterpart pin the formula for wave32 and wave64, including a misaligned offset (0x41).259ffee6bed, 1,086 focused tests pass, 183 are skipped, and none fail. This covers the layout, signed offsets, private-atomic rejection, wait planning, and BUFFER and translated-memory paths. All four new CDNA1–4 canary tests fail with the original0c70fdcdd2batomic guard and pass with the fix. Release Clang 20 linking needed a build-only explicit template instantiation for the existing missingComputeUnitCore::step_impl<true>symbol; the workaround is not part of this PR.45fac8cfd27only corrects citation comments. The full suite and performance measurements have not been rerun for these later fixes.4294f43c632was checked with 1,309 selected tests: 1,126 passed, 183 skipped, and zero failures. This reruns the focused scratch, FLAT, wait-planning, address-calculation, atomic, BUFFER, GPU VM pipeline and execution-harness coverage, and adds all SpillManager and routed-memory observation tests. The changed test translation units were rebuilt; linking used the same build-only explicit instantiation described above. Formatting and whitespace checks pass. The revised spill-slot test checks allocated logical offsets against explicit wave32/wave64 interleaved backing addresses through the production mapping helper; it does not claim to validate trampoline encoding. Plugin field comments now describe interleaved-layout metadata, including global BUFFER swizzling, without implying private backing provenance. No full-suite, performance or hardware rerun is claimed.The layout change has not been checked on real hardware; ordinary scratch round trips do not reveal the layout. Two checks on an R9700 would confirm it. The first is to write lane IDs to scratch and read them back through a global alias of the scratch backing. The second is to print a private variable with ROCgdb and compare it with the emulator.
Known regression: threefry4x32
threefry's only scratch instruction is
scratch_store_b128. Each lane's 16-byte store used to be one write, and it is now four dword writes. Scratch reachesL1VectorCache::store_swizzled, but its inner cross-lane dword batch is skipped becausecached_private_ram_mtypesupplies no eligible policy for the scratch backing. Both the policy hint and the eventual batch writer requireDriverSealedRamownership. Enabling scratch batching is a proposed follow-up, not a validated fix: it must satisfy both ownership checks and preserve backing safety, then be checked for correctness and measured against this regression. Changing the hint alone does not establish that the batch will run or recover the lost performance.Review focus
SWIZZLE(offset, ThreadID)wording and on rocm-dbgapi, not on an explicit formula. Please flag it if you know of RDNA4 scratch behaving differently from RDNA3.