Repository navigation
fix(recompiler): emit a formatted buffer access through one function per resource - #1959
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Linux CI finished with 497/498 tests passing. The new agc_formatted_buffer_once test fails at its first assertion: it emits 0 format switches for 3 loads, while the test expects one shared switch. This suggests the fixture is not exercising the generic prepared-descriptor path described in the PR, or the expected SPIR-V shape is wrong. Please fix the fixture or assertion and rerun the full checks; I will re-review the updated head.
86780e6 to
565deb2
Compare
|
Thanks, The fixture was fine, my assertion was the problem. CI builds with SPIRV-Tools on, and there the specialized module goes through dead-branch elimination, which folds the format switch away completely. So it ends up with 0 switches whether there's 1 load or 3. I only ran it locally without SPIRV-Tools. The shared function is still there either way (one call per load, same number of functions), so I changed the test to check that instead: 3 loads shouldn't add any switches or functions compared to 1 load, just one call each. It passes with and without SPIRV-Tools now, and if I put the inline switch back it fails in both. |
MarcZX10
left a comment
There was a problem hiding this comment.
All checks now pass, but the updated test still permits the original failure mode: switches <= 1 accepts zero switches. It also compares only one-load and three-load outputs, so equal function counts do not establish that either output contains the shared helper; the two additional calls alone do not prove the first load took that path. Please add a zero-load baseline and assert that one dynamic formatted load emits the expected helper and format dispatch, while three loads add two calls but no additional helper or dispatch. Then rerun the checks. I will re-review the result.
MarcZX10
left a comment
There was a problem hiding this comment.
Thanks for clarifying that SPIRV-Tools folds the format switch away; I withdraw the requirement to assert a switch. One coverage gap remains: comparing only 1-load and 3-load modules proves the two extra loads add two calls, but does not prove the first load itself calls the shared helper. Please include a zero-load baseline and assert that 1 load adds one call, then 3 loads add two more, with no extra helper functions. That will verify every load uses the shared function both with and without SPIRV-Tools.
565deb2 to
3870a9d
Compare
|
Thanks again. I added a baseline with no formatted loads, so now one load has to add exactly one call and one function, and three loads only add two more calls with no extra function or switch. Passes with and without SPIRV-Tools. I also checked it actually catches the thing u pointed out, made only the first load emit the switch inline and the rest call the helper, and the test fails in both builds ("one formatted load emitted 0 calls and 1 functions more than none"). |
MarcZX10
left a comment
There was a problem hiding this comment.
The zero-load baseline closes the remaining coverage gap: one load must add one call and helper, and two more loads add two calls without another helper. The reported inline-first-load mutation fails this assertion, which checks the intended regression directly. I re-reviewed the full diff; Linux, Windows and Conventions pass on this head. Approved.
3870a9d to
857cdec
Compare
|
The relinker (windows-latest) failure here isn't from this PR: main fails the same way at 3bd74c1. |
857cdec to
af6d40c
Compare
af6d40c to
f5e118a
Compare
|
Native Windows run, since no Windows results were posted. Setup: Windows 10 (19045), RTX 3060 Laptop (NVIDIA 616.92), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release. Head
No title was run. 🤖 Addressed by Claude Code |
…per resource A formatted buffer load or store whose format comes from a prepared descriptor emitted the switch over every buffer format inline, at each access. The switch is now emitted once per resource, component count and direction as a SPIR-V function taking the index, offsets, record count and storage buffer offset, and each access calls it. Specialization still folds the switch to the descriptor's format. Accesses through GPU-selected descriptors keep the inline switch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f5e118a to
f2bd6d4
Compare
|
Checked on Linux (RADV): |
What
Since #639 a shader is compiled once to a generic module and specialized per descriptor at its first draw or dispatch. A formatted buffer load or store whose format is a specialization constant (a prepared descriptor) emitted the switch over all 77 buffer formats inline at every access, so a shader with many formatted accesses produced a very large generic module, and specializing it at first use took long.
SpirvModulegets a second function section (BeginHelperFunction/EndHelperFunction) thatFinalizeappends after the entry point.On the console the buffer unit decodes the V# and does the addressing for each access in hardware, so one implementation serves every access; this emits the addressing once per resource in the same way.
PPSA36082
One compute shader has 16 formatted loads from one descriptor and 8 formatted stores to another. Its generic module was 2,851,193 words, with 99.8% of its instructions inside the 24 inlined switches, and its specialization took about 200 ms on the queue worker at about 3.6 s, while the loader waits on a GPU fence before it frees its 512 MiB upload pool. With this change the generic module is 245,872 words, the specialized one 51,338 (was 681,294), and the specialization takes about 16 ms.
On main f145139 with #861, #1367, #1249, #1905, #1961 (import at map) and #1960 (imports leave pages unprotected) it reaches the main menu in 10/10 runs (40 s each); before this change, on the same stack without #1960, 0/4: the pool overflows at about 3.7 s; two stack snapshots and a
perfprofile of failing runs put the queue worker inSpecializeSpirvfor this shader.Tested
Linux (CachyOS), x86-64, GCC, 7800X3D / RTX 4090.
agc_formatted_buffer_once(new, no device): against a shader without formatted loads, one formatted load adds one call and one function; two more loads from the same descriptor add two calls, no function, no format switch and under 10% to the module. It passes with and withoutANYPS5_ENABLE_SPIRV_TOOLS(with it, the specialized switch is folded away). A formatted store to a second descriptor gets its own function, and two more loads and stores add only calls. The function is marked not to be inlined. Emitting the switch inline at every access, or only at the first, or leaving the function inlinable, fails it in both builds.agc_driver_buffer_format,agc_driver_format_d16andagc_driver_format_d16_storerun the new function on the device (loads and stores, 1 to 4 components, d16) and pass.spirv-val(Vulkan 1.3) on the 37 modules these tests produce: all valid.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)