Repository navigation
fix(shader): read quad_perm DPP lanes in pixel shaders through quad operations - #6290
jackthereaper2006 wants to merge 2 commits into
Conversation
|
Windows and NVIDIA run, for the "GPUs tested" line (RADV and lavapipe on Linux only). Windows 11 (26200), RTX 3060 Laptop GPU (NVIDIA 617.42), WinLibs GCC 15.2.0 r7 (UCRT), Release,
Not checked: which of the GPU tests above run a pixel shader with a DPP AI-assisted: yes (Claude Code) |
|
Windows with AMD's own driver, the combination not covered yet: the Showcase artifact from #6289 shows up there on Setup: Windows 11 Home 26300, AMD Radeon 860M (RDNA 3.5, Title
Tests
Not tested: discrete RDNA, Linux. AI-assisted: yes (Claude Code) |
|
An Intel run, for the "Not run: Intel" line. It passes, but Gen9 does not show the bug on Setup: Intel HD Graphics 520 (Skylake GT2, Gen9), Mesa 25.2.8 ANV, Ubuntu 24.04 (container), kernel 6.12.111, GCC 13.3, Release. Head
So on ANV Gen9, as on lavapipe, the shuffle path already reads helper neighbours correctly, and the PR keeps it working. The test does not catch the RADV failure on this device. I could not run the PS5 OpenGL Showcase here. AI-assisted: yes (Claude Code). The runs are on my machine. |
klosixgaming-arch
left a comment
There was a problem hiding this comment.
Merged head 3dd3a9832 onto current main (40393b0f9) — clean merge, builds with WinLibs GCC 15.2.0, Release, Ninja, -j 8. Windows 11, NVIDIA GeForce RTX 5090 (selected device; RTX 4090 and an AMD iGPU are also present), 32-lane subgroups.
Tests run on the merged tree:
agc_shader_memory— pass (includes the new pixel quad_perm check)agc_driver_pixel_quad_perm_helpers— pass- DPP/quad regression set, all pass:
agc_dpp_write,agc_driver_dpp8,agc_driver_dpp8_pk_fmac,agc_driver_dpp16_inactive_source,agc_driver_dpp16_operands,agc_driver_dpp16_row_share_xmask,agc_driver_dpp_no_form,agc_driver_quad_pixel_mask,agc_driver_vop3_quad_sad - Sanity:
request_serializer,agc_driver_gpu_buffer_format,agc_driver_layered_volume_draw— pass
Negative control (plain main 40393b0f9 plus only this PR's test changes):
agc_shader_memoryfails with "a pixel quad_perm read did not go through quad operations"agc_driver_pixel_quad_perm_helpersskips (exit 77, "skipped, the device has no subgroup quad operations") because main does not exposeGroupNonUniformQuad
Note: on NVIDIA the exec test passes even without the emitter change (NVIDIA keeps helpers in the shuffle), so on this machine the failing negative control is the agc_shader_memory check; the RADV-specific failure was already demonstrated by @jackthereaper2006 and @LeandroLP23 above.
Read through the diff: capability exposure in VulkanDevice.cpp, validation mapping in ShaderValidation.cpp, and quad-broadcast emission for pixel-stage quad_perm reads (control ≤ 0xff) covering both the DPP move value and the write-condition source EXEC check — no problems found. Not tested here: AMD, Intel, Linux, titles.
AI-assisted: yes. Written with AI assistance and reviewed by me.
…perations A DPP quad_perm read in a pixel shader, such as the neighbour read of a derivative, was emitted as a subgroup shuffle with a ballot of EXEC. Mesa gives a fragment shader full quads only for quad group operations, or for subgroup operations when it requires full quads (nir_gather_info.c), so on RADV the shuffle and the ballot ran without helper invocations and reads from helper lanes were wrong. When the device supports quad operations, a pixel quad_perm read and the source EXEC check of a DPP write now use OpGroupNonUniformQuadBroadcast. Other DPP controls, other stages and devices without quad operations keep the shuffle. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Draws triangles whose quads have helper pixels, turns whole-quad mode on and reads the horizontal and vertical neighbour's pixel index through v_mov_b32_dpp quad_perm, with and without fi, in wave64 and wave32. Every covered pixel must read its neighbour's index, helper or not. On RADV the shuffle path read 0 from helper neighbours. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3dd3a98 to
5e25bc8
Compare
|
Test run (not a code review). I merged this PR onto current
So: no regression found, and no evidence of a fix either way. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 497 tests)Skipped (45)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
OK 1. Freshness Envelope
2. Findings
3. Cross-PR Ledger
|
|
Thanks for this; it fixes the case we hit too (helper neighbours read through One neighbour read stays on the shuffle path here: A wider coverage note, not a regression report: in a helper lane, the value and the source EXEC a quad read uses, and whether the lane runs a block at all, can still come from operations Vulkan lets helper invocations stay inactive for: an EXEC built from compare masks ( We have a follow-up on top of this branch, held until this merges: |
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — approved
Pixel-shader DPP quad_perm reads now go through OpGroupNonUniformQuadBroadcast when the device has quad operations. The change merges cleanly and builds. The focused tests pass, and the new agc_shader_memory check fails once the production files are reverted to main.
What I ran
Head 5e25bc870e6eda3f0cb994290a53f71cac0aaf8d, main 1c8c0efc8c41314402e6966da5bef6ea3d639d10, merged tree 2c00d3afe895b0453533a9f7e4019b234ca50c7d. The machine is Debian 13 (VM, 32 vCPU Xeon Gold 6136) with GCC 14.2.0, and Vulkan runs on lavapipe (Mesa 26.1.6, LLVM 19, 8-lane subgroups).
git diff --check 1c8c0efc8c41314402e6966da5bef6ea3d639d10...5e25bc870e6eda3f0cb994290a53f71cac0aaf8d
python3 tools/check_conventions.py --base 1c8c0efc8c41314402e6966da5bef6ea3d639d10
ctest --test-dir build -j8 --test-load 32 --output-on-failure -V -R "^(agc_driver_pixel_quad_perm_helpers|agc_shader_memory|agc_dpp_write|agc_driver_quad_pixel_mask|agc_driver_dpp16_inactive_source|agc_driver_pixel_helper_lanes|agc_shader_quad_mask_no_demote)$"
# negative control
git show 1c8c0efc8c41314402e6966da5bef6ea3d639d10:<path> > <path> # the 5 production files
ninja -C build -j32 -l32 agc_shader_memory_tests
build/tests/agc_shader_memory_tests
git show 2c00d3afe895b0453533a9f7e4019b234ca50c7d:<path> > <path> # restore
ninja -C build -j32 -l32 agc_shader_memory_tests agc_driver_pixel_quad_perm_helpers_tests
build/tests/agc_shader_memory_tests
git status --porcelain
Results
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
Configure, build, libs (script) |
ok |
| Selected tests (script, 2177 run) | 0 new failures, 0 timeouts. The 7 known failures (agc_driver_division, denormal-mode tests, vector_constant_factor_tininess, an unwrap test) also fail on main |
| Skips (script) | 117, all expected per check_skips.py |
| Focused ctest (7 tests) | 4 pass: agc_shader_memory, agc_dpp_write, agc_driver_quad_pixel_mask, agc_shader_quad_mask_no_demote. 3 skip on lavapipe with "subgroup size 8 cannot hold a wave32": agc_driver_pixel_quad_perm_helpers, agc_driver_pixel_helper_lanes, agc_driver_dpp16_inactive_source |
| Validation layer (script) | layer loaded in 784 of 785 tests, missing in none. The only test with VUIDs not seen on main is agc_driver_image_gather_volume (VUID-VkSamplerCreateInfo-addressMode*). That test is marked "test not run on main", so the baseline has no comparison for it. It checks sampler address modes, which this change does not touch, so I don't count it against the PR |
| Negative control | fails as expected (see below) |
| Worktree after restore | git status --porcelain is empty |
agc_driver_pixel_quad_perm_helpers on lavapipe
1415: Selected GPU name=llvmpipe (LLVM 19.1.7, 256 bits) vendor=0x10005 device=0x0 subgroup=8
1415: skipped, subgroup size 8 cannot hold a wave32
3/7 Test #1415: agc_driver_pixel_quad_perm_helpers ...***Skipped 0.10 sec
Negative control
I restored these five production files from main:
VulkanDevice.cppShaderValidation.cppSpirvSubgroup.hppSpirvSubgroup.cppSpirvAluEmitterMath.cpp
I kept the PR's tests and rebuilt agc_shader_memory_tests. It fails with:
a pixel quad_perm read did not go through quad operations
After I restored the merged files, it passes again.
The GPU execution test agc_driver_pixel_quad_perm_helpers skips on this machine, so I couldn't run a negative control for it. Others ran it in the discussion:
- It tells the two paths apart on RADV (the author) and on AMD's Windows driver (LeandroLP23).
- On NVIDIA (screamm) and ANV Gen9 (odntht) it passes with the shuffle path too, so it can't tell the paths apart there. I can't add anything new on that point.
Findings
Blocking
None.
Non-blocking
- The quad read emits more code than it needs to (
core/shader/recompiler/SpirvBackend/src/SpirvMemory/SpirvSubgroup.cpp:317-329, used atcore/shader/recompiler/SpirvBackend/src/SpirvAlu/SpirvAluEmitterMath.cpp:1382-1386and:196).- What it does now: each read emits four
OpGroupNonUniformQuadBroadcastplus three selects. A move withoutfiemits that twice, once for the value and once for the source EXEC. That is why screamm counted 60 broadcasts where there were 15 shuffles before. - Why it's wasteful:
controlis a compile-time constant. When all four entries are the same (0x00, 0x55, 0xaa, 0xff), oneOpGroupNonUniformQuadBroadcastwith that index is enough.[1,0,3,2],[2,3,0,1]and[3,2,1,0]map directly toOpGroupNonUniformQuadSwapwith direction 0, 1 and 2. - Impact: it's only code size and ALU work, not a correctness issue. LeandroLP23 measured no change in FPS. It could go in a follow-up.
- What it does now: each read emits four
- The point that
QuadBroadcastof EXEC is undefined when the source invocation is inactive was raised by screamm and answered by the author. I checked that pixel shaders keep the scalar control flow uniform here, and I have nothing new to add. Astronaut10's coverage notes onrow_xmaskand reconvergence are follow-up scope.
Not verified
- The GPU execution path itself. On lavapipe,
agc_driver_pixel_quad_perm_helpersskips because of the 8-lane subgroup, so I didn't run any pixelquad_permread on a GPU here. The RADV, AMD Windows, NVIDIA and Intel results come from the discussion, not from my runs. - The PPSA99005 title result. I did not launch any title.
- The validation-layer comparison for
agc_driver_pixel_quad_perm_helpers. It skipped, so it reached no draw.
|
Thanks @XairRrR for the review and the negative control, and @Astronaut10 for testing on RDNA4. row_xmask: you're right that this PR leaves it out. The quad path only takes Fewer broadcasts: agreed. |
What
In a pixel shader, a DPP
quad_permread is now emitted asOpGroupNonUniformQuadBroadcastfrom each lane of the quad, with the result chosen by the lane'squad_permentry. This covers the source EXEC check of a DPP move and of a DPP write condition as well as the value. Other DPP controls, other stages and devices withoutVK_SUBGROUP_FEATURE_QUAD_BITkeep the subgroup shuffle and ballot.Until now the read was a subgroup shuffle plus a ballot of EXEC. Mesa gives a fragment shader full quads only for quad group operations, or for subgroup operations when the shader requires full quads (
RequireFullQuadsKHR) (nir_gather_info.c,needs_full_quad_helper_invocations). On RADV the shuffle and the ballot therefore run without helper invocations, and a DPP read from a helper lane is wrong. The derivatives that ACO builds from DPPquad_permreads are the visible case.The same failure shows on AMD's Windows driver (LeandroLP23's run). NVIDIA, Intel ANV Gen9 and lavapipe already keep helpers in the shuffle, so on those drivers the change only moves the read to quad operations.
The device now lists
GroupNonUniformQuadwhen it reportsVK_SUBGROUP_FEATURE_QUAD_BIT, and shader validation maps that capability to the bit.Tested
Linux (CachyOS, kernel 7.2.9), GCC 16.2.1, Release,
BUILD_TESTING=ON. Rebased onmain6ef5a1c4c; the only conflict wasShaderMemory.cpp, whereverifyFmaskImageModeandverifyPixelQuadPermReadswere added at the same place. Runs that name no commit were onmain0afa6a607before the rebase.37a0ef4e…d388). Its instance pixel shader takes the normal fromnormalize(cross(dFdx(v_world), dFdy(v_world)))(main.c:259). In the translated program0x229255000that becomes the only shader with DPP moves.RADV on
main: the 8192 instances render as a white bloom cloud with black specks.RADV with this change: they render as in the author's screenshot.
Checks on
main:APS5_SKIP_SHADER=229255000the rest of the frame is correct.RADV_DEBUG=nodcc,nofastclearsandzerovrameach leave the cloud.Frames (
APS5_DUMP_FRAMESdumps of the title's own display buffer, 960×540):main, RADVmain+ the fix, RADVmain, lavapipemain, RADV,APS5_SKIP_SHADER=229255000Full-size files: https://gist.github.com/jackthereaper2006/aec623dfe08f2a6d8e5207ac3e2a209b. Reference: the author's screenshot.
agc_shader_memorycheck:quad_permreads, with full and partial row masks, useOpGroupNonUniformQuadBroadcastand no shuffle.agc_driver_pixel_quad_perm_helpers(5e25bc8, 3dd3a98 before the rebase): pixel quad_perm DPP reads of helper neighbours afters_wqm, with and withoutfi, wave64 and wave32. It passes on RADV, fails on RADV with the emitter frommain(pixel 584 read 0 from quad_perm [1,0,3,2], its helper neighbour is pixel 585), and skips on lavapipe (8-lane subgroup) and without the quad capability. Wholectestwith it: 1227/1227 with Mesa patched for agc_driver_tessellation segfaults on RADV with Mesa 26.2+: nir_opt_varyings corrupts the TCS input offset's use list (Mesa bug) #5444.main6ef5a1c4c,ctest -V -R 'quad|dpp|agc_shader_memory' -E audio_outwithVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1,ANYPS5_REQUIRE_VULKAN=1, validation layers 1.4.363):main: 11/11 pass, noVUID-errors.agc_driver_pixel_quad_perm_helpers), noVUID-errors.was not found but was requested by env var VK_INSTANCE_LAYERS;VK_LOADER_DEBUG=layershows the layer inserted at instance and device level.ctestonmain6ef5a1c4c, RADV, stock Mesa 26.2.4: 1776 pass, 32 skip and 7 fail out of 1815. The 7 failures fail the same way with this change's files reverted tomain:agc_driver_tessellation: the Mesa crash of agc_driver_tessellation segfaults on RADV with Mesa 26.2+: nir_opt_varyings corrupts the TCS input offset's use list (Mesa bug) #5444.agc_driver_log_near_one,agc_driver_vop1_float_unary,agc_driver_fp16_overflow,agc_driver_float16_result_modifiers: they passed on0afa6a607on this machine (e.g.v_log_f32of 2.0 now returns 25.0).agc_driver_pixel_sample_iteration,agc_idle_publish_wait: new onmainsince0afa6a607.ctestonmain0afa6a607:agc_driver_tessellation, the Mesa crash of agc_driver_tessellation segfaults on RADV with Mesa 26.2+: nir_opt_varyings corrupts the TCS input offset's use list (Mesa bug) #5444.ctest -R '^agc_'on lavapipe: 573/573.maind92a8dad3plusagc_import_watch_other_family, which is new onmainand skips because this device has no other queue family.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)0x1638), Mesa 26.2.4, CachyOS Linux 7.2.9; lavapipe, Mesa 26.2.4 (built locally), same machine🤖 Generated with Claude Code