Repository navigation
fix(shader): widen LDS atomic fences to device scope on AMD's proprietary driver - #8489
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
98f4019 to
c1528a6
Compare
|
Independent Windows run of this PR on an RDNA 2 card (data only). Environment: Windows 10 Pro 10.0.19045, Ryzen 7 5800X3D, AMD Radeon RX 6400 (AMD driver 32.0.21033.3005; the log shows Results (3 runs each):
So the stale LDS read the PR describes reproduces on an RX 6400 without the device-scope fence and is gone with it. I did not inspect the driver's code objects, so I can not confirm the Not run: PS5LM Plus or any title, the replay comparison against NVIDIA, other GPUs and platforms, the full repository CTest. AI-assisted (Claude Code). |
c1528a6 to
f1a2103
Compare
|
Rebased onto Windows 11, WinLibs GCC 15.2.0,
Thanks @xXPBSXx for the RX 6400 run. AI-assisted: yes (Claude Code); results above were run on this machine. |
…tary driver The recompiler orders an LDS atomic, and the compare-exchange loop that writes a byte or a halfword of LDS (ds_write_b8/b16), with a workgroup-scope OpMemoryBarrier after the update. AMD's proprietary Windows driver 32.0.21042.62 (Radeon 0x13c0, RDNA 2) compiles that memory barrier to s_barrier, as the code objects in its pipeline cache show. Such updates usually sit in branches that only some waves of a workgroup take, so those waves execute extra s_barriers, which pair with the other waves' barriers, and the other waves read LDS before it is written. An OpMemoryBarrier is not an execution barrier and SPIR-V allows it in non-uniform control flow, so the SPIR-V is valid; a device-scope OpMemoryBarrier compiles to GL0 and GL1 cache invalidations without s_barrier there. PS5LM Plus (PPSA99581, llama.cpp's Vulkan backend) drifted into garbage on that driver with flash attention on. Its flash-attention shader stores two halfwords to LDS in a branch that two of its four waves take, inside the KV loop. The driver's code had 23 s_barrier for the guest's 21; a replay of the dispatch differed from NVIDIA in 159 words, up to 1.7e38, and now has 21 and differs only in the last bit of 9 words. On that driver the fence is now device scope, which orders everything the workgroup-scope fence did. Other drivers keep the workgroup scope. The flag is part of the shader cache key, and agc_shader_replay takes --device-scope-lds-memory-barriers to replay a request the way that driver compiles it. The LDS update helper already fences, so the second, identical fence after it in ds_min/max_f32 and the other read-modify-write LDS operations is dropped. agc_driver_lds_subword_store_branch stores a halfword to LDS in a branch that waves 0-1 of four take, delays those waves and then exchanges LDS slots across one s_barrier. Before, thread 64 read a stale slot on the AMD driver; it passes on AMD and NVIDIA now. agc_subgroup_barrier checks the driver quirk and that ds_write_b16, ds_add_u32 and ds_max_f32 each emit one fence of the expected scope. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BM2kfDbr6MydxsfTckZJDR
f1a2103 to
98df163
Compare
|
Checked the diff, the new tests and the third-party AMD run. The fence scope only changes for |
Builds on #8134, which has merged; this branch is now a single commit on
main.What
The recompiler puts a workgroup-scope
OpMemoryBarrier(Workgroup, AcquireRelease | WorkgroupMemory)after three kinds of LDS update:ds_write_b8/ds_write_b16);AMD's proprietary Windows driver compiles that memory barrier to a real
s_barrier. I read the driver's code objects out of its pipeline cache to see this; capturingVK_KHR_pipeline_executable_propertiesinternal representations fails with -13 for this pipeline.These updates usually sit in branches that only some waves of a workgroup take. Those waves then execute extra
s_barriers, which pair with the other waves' real barriers, so the other waves read LDS before it is written.An
OpMemoryBarrieris not an execution barrier, and SPIR-V allows it in non-uniform control flow. The SPIR-V is valid, so this is a driver bug. On the same driver, a device-scopeOpMemoryBarriercompiles tobuffer_gl0_inv+buffer_gl1_invwith nos_barrier.PS5LM Plus (PPSA99581, llama.cpp's Vulkan backend) drifted into different garbage on every run on that driver with flash attention on. Its flash-attention shader stores two halfwords to LDS inside the KV loop, in a branch that waves 0-1 of its four take. Results from replaying that dispatch with identical inputs:
s_barrierin the driver's code (guest has 21)The guest's 21
s_barriers map to 21OpControlBarriers, which the driver compiles to ones_barriereach, none moved or duplicated. The 2 extra come from the two halfword stores.Changes:
WorkgroupScopeFenceUsable(VkDriverId)inSubgroupBarrier.hppis false only forVK_DRIVER_ID_AMD_PROPRIETARY. NVIDIA, RADV, AMDVLK, Intel, llvmpipe and MoltenVK keep the workgroup scope. AMDVLK is excluded only because it was not tested.SpirvTarget::deviceScopeLdsMemoryBarriers, which is part of the shader cache key.LdsMemoryBarrierScope) is device scope, with the same semantics. Device scope includes the workgroup, so it orders everything the old fence did. Modules use the GLSL450 memory model, and device-scope fences are already emitted for buffer atomics.SharedAtomicUpdate(ds_min/max_f32,ds_cmpst_f32and the other read-modify-write LDS operations) emitted a second fence identical to the oneAtomicUpdatealready emits. The duplicate is dropped, so each update has one fence on every driver.agc_shader_replay --device-scope-lds-memory-barriersreplays a request the way that driver compiles it. Like--wave-lds-memory-barriers, the flag is not serialized.docs/dev/debt/shader.md.The fences inside the LDS lock path are untouched, because that path emits none. On that driver a
ds_add_u64program (the lock path) had nos_barrierbesides its uniform lock-initialisation barrier, and an LDS exchange after it in a branch passed.Not covered:
Tested
Windows 11, WinLibs GCC 15.2, full Release build of 98f4019 (on #8134 473497c, main 3aa35dc; the branch also merges cleanly with main 44e3389).
ctest -R "agc|shader|spirv|recomp"ran withANYPS5_REQUIRE_VULKAN=1and-j4; failures were rerun alone. The baseline is #8134's head 473497c.agc_driver_msaa_dcc_clear_4andagc_driver_pass_input_hazard. The baseline has the same 2 of 826.agc_trimmed_import,agc_driver_msaa_dcc_clear_4,agc_driver_draw_cache_reuse_offandagc_driver_stencil_replay_no_gplalso fail on the baseline.agc_driver_layered_volume_vertex_no_gplfails with a device fault reading0xbeefdeadc000, a fault the debt file already lists for this driver. The family's failures depend on test order: on the baseline the same fault hitsagc_driver_layered_volume_drawandagc_driver_layered_volume_vertexinstead (baseline: 8 of 826 failed).Linux (WSL Ubuntu 24.04, GCC 13.3, llvmpipe): full build with
-DBUILD_TESTING=ON, the same ctest set, 817 tests. The same 17 tests fail as on #8134; they need watched guest memory or llvmpipe features.agc_subgroup_barrierandagc_shader_disk_cachepass, andagc_driver_lds_subword_store_branchskips (no 32-lane subgroups).New and changed tests:
agc_driver_lds_subword_store_branchruns four wave32 waves. Waves 0-1 store a halfword to LDS in a branch and then spin for 4096 iterations. After that, every thread writes an LDS slot, passes ones_barrierand reads the slot of threadtid ^ 64. The test runs 16 rounds.round 0: thread 64 (wave 2) read 868449278, expected 65536.agc_subgroup_barrieralso checks the new quirk, and thatds_write_b16,ds_add_u32andds_max_f32each emit exactly one fence of the expected scope with the flag off and on. It fails without the change.agc_shader_disk_cachechecks that the new flag changes the cache key.PS5LM Plus v0.8.4 was run with
qwen2.5-0.5b-instruct-q4_k_m.gguf,-fa on -ngl 999, the prompt "The capital of France is" and temperature 0, on the AMD iGPU with a warmed shader cache per configuration. Flash attention also needs #7410: without it, NVIDIA too answers garbage with-fa on. So these runs use a local integration branch, this PR (#8134 + this commit) merged with #7410 (a80f17d). The "before" rows are the same build with the new flag forced off." Paris. It is not a city and not to\",__ F 12G"" Paris. It is the largest city in Europe and the second largest in the world"" Paris. It is in France, in France and or in France, in France"The correct text matches NVIDIA and the CPU. The first cold q8_0 run of the final commit hit the guest's own 10 s fence timeout while shaders compiled. The previous revision of this commit, with the duplicate fence still in place, gave the same correct text on 5 q8_0 and 4 f16 runs.
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)🤖 Generated with Claude Code
https://claude.ai/code/session_01BM2kfDbr6MydxsfTckZJDR