Repository navigation
perf(recompiler): keep relatively addressed vector registers in a window array - #3856
Conversation
…dow array `v_movrels_b32`, `v_movreld_b32`, `v_movrelsd_b32`, `v_movrelsd_2_b32` and `v_swaprel_b32` lowered to a compare-and-select chain over every register from the base to the program's last allocated register, so a program addressing 154 registers 104 times turned 3176 ISA words into 649,515 SPIR-V words that Intel's compiler did not finish. The registers from the lowest relative base up now live in a function-scope array per invocation (`LoadVectorWindow`/`StoreVectorWindow`), so an indexed access is one load or store; plain accesses to those registers go through the same array, DPP-modified relative writes keep their `DppUpdateU32` merge, an index past the window reads 0 and drops its write, and programs without relative addressing are unchanged. The same program recompiles to 392,333 words with 4,992 selects instead of 32,383. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Linux results for this commit, from #3842: AI-assisted: yes (Claude Code). |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for carrying this, and thanks @Mouad-Scriptz-0 for the Arc and Killzone runs. The window array itself looks good: a build and the scoped ctest pass here (405 tests), including agc_vector_window, agc_driver_relative_indexing, agc_driver_scalar_relative_indexing and agc_driver_dpp16_operands. The problem is the out-of-range behaviour the PR adds. We measured it on RDNA2 hardware (gfx1035, 40 VGPRs allocated, a different v0 in every lane), and it doesn't match what the PR does:
- An out-of-range read returns
v0, not 0. "Out of range" means base + M0 ≥ the VGPR allocation, and the write is dropped. For example, withv_movrels_b32 v10, v32, any M0 ≥ 8 readsv0. The PR reads 0 (ControlFlowInstructions.cpp:318,Select(inRange, loaded, 0)). So the TechnicalDebt line ("reads 0 ... as the RDNA ISA describes") is wrong. - M0 is unsigned. M0 = 0xffffffff, 0xfffffffe, 0x80000000 and so on are all out of range:
v0is read and nothing is written. The PR computesraw = (reg - windowBase) + M0in 32 bits (:307). When the base register is above the window start, M0 = -1 wraps back into the window, so the PR reads or writes the register below the base. v_swaprel_b32with either side out of range writes nothing, to the destination or to the source. The PR still writes the value it read out of range into the in-range side.v_movrelsd_b32/_2with only the source out of range readsv0and does write an in-range destination.
To check this against the PR's own code, we appended these rows to tests/execution/RelativeIndexing.cpp (window v30..v37, lane value x = 0x1000·(lane+1)). The HW and PR columns are for lane 0, and the pattern is the same for every lane:
| rows appended after the existing program | HW (gfx1035) | this PR |
|---|---|---|
s_mov_b32 m0, -1 / v_movrels_b32 v21, v32 → v21 |
v0 (= lane id, 0x0) |
0x1001 (v31) |
v_movreld_b32 v33, v4 → then v32 |
0x1030 (unchanged) | 0x1000 |
v_movrelsd_b32 v35, v34 → then v34 |
0x1002 (unchanged) | 0x1020 |
| → v33 | 0x1020 | 0x1020 |
s_mov_b32 m0, 0x103ff / v_swaprel_b32 v31, v32 → v32 |
0x1030 (unchanged) | 0x0 |
main doesn't get these right either: its select chain falls back to the base register. But this PR replaces that with another model and documents it as the ISA's. What would fix it:
- In
relativeWindowAccess, check the range on M0 itself:offset < windowSize - (reg - windowBase), unsigned. The clamped index can stay as it is. - Read
v0when the index is out of range (GetVectorReg(0), which goes through the window when v0 is in it). Onlyv_swaprel_b32writes, and only when both sides are in range. - Initialise the window array (an
OpConstantNullinitializer on theOpVariable). An unwritten register currently reads an undefined SPIR-V value; onmainthe SSA builder gives it 0. - Add the rows above, with their HW values, to
RelativeIndexing.cpp, plus a row where the source is out of range and the destination is in range (v_movrelsd_2_b32with M0 = 0x000103ff: the destination getsv0). Add the same rows to a wave64 test as well, since the window is one array per lane half.VectorWindow.cpponly counts IR shapes, so it can stay as a regression guard for the select count, but the behaviour needs value rows. - Update TechnicalDebt.md:580. Say that out-of-range reads return
v0and writes are dropped, measured on gfx1035. Say that the window ends at the last register the program names rather than at the shader's VGPR allocation, which the recompiler doesn't know. So a relative write past the named registers but inside the allocation is dropped, while hardware keeps it and a later relative read would see it. Also give the actual size (649,515 → 392,333 words) instead of "noted in its pull request". - Remove the comment above
RelativeWindowAccess(ControlFlowInstructions.cpp:296-297). It describes the select chain this PR replaces, and CONVENTIONS asks for no comments in code.
…red semantics M0 is unsigned, so an index at or past the window reads v0 and drops its write, v_swaprel_b32 writes neither side when either is out of range, and the window array starts zeroed like unwritten SSA registers; the window load counts as a lane source so a wave64 program's second half keeps its own registers. agc_driver_relative_indexing runs in wave32 and wave64 and pins the rows measured on gfx1035 in the review of boykopovar#3856, and the TechnicalDebt entry states the measured behaviour, the window's end at the last named register and the sizes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Two more agc_driver_relative_indexing rows read a register of the window the program never wrote, directly and through v_movrels_b32, and expect 0, which the zeroed window array gives like an unwritten SSA register did; the TechnicalDebt entry says the console's value is undefined. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks for the measurements. Fixed on
Tested on Fedora 44 / Intel Arc Meteor Lake (Mesa 26.2.3), all three commits: AI-assisted: yes (Claude Code). |
|
Updated with @Mouad-Scriptz-0's commit addressing the review: M0 is unsigned, out-of-range reads return v0, v_swaprel drops writes when either side is out of range, the window array is zero-initialized, and the gfx1035 test rows and TechnicalDebt updates are added. Conventions check passes. |
adriadam10
left a comment
There was a problem hiding this comment.
Checked the measured out-of-range semantics (unsigned M0, out-of-range read returns v0, writes dropped, v_swaprel_b32 writes neither side unless both are in range, zero-initialised window), agc_vector_window and agc_driver_relative_indexing (wave32 and wave64) on the PR merged onto main, and that both fail without the change. Thanks @Mouad-Scriptz-0 for the Arc/Killzone runs. Merging once CI is green.
What
Carrying @Mouad-Scriptz-0's fix from #3842 (commit f8ed012).
v_movrels_b32,v_movreld_b32,v_movrelsd_b32,v_movrelsd_2_b32andv_swaprel_b32lowered to a compare-and-select chain over every register from the base to the program's last allocated register, so a program addressing 154 registers 104 times turned 3176 ISA words into 649,515 SPIR-V words that Intel's compiler did not finish. The registers from the lowest relative base up now live in a function-scope array per invocation (LoadVectorWindow/StoreVectorWindow), so an indexed access is one load or store; plain accesses to those registers go through the same array, DPP-modified relative writes keep theirDppUpdateU32merge, an index past the window reads 0 and drops its write, and programs without relative addressing are unchanged. The same program recompiles to 392,333 words with 4,992 selects instead of 32,383.Fixes #3842.
Tested
python tools/check_conventions.py --base origin/mainpasses.main(a11c8f7f7).Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)