Repository navigation
fix(shader): release the LDS lock with an exchange and drop fences inside it - #6976
Conversation
|
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.
|
4b3d66a to
a663dfc
Compare
|
RDNA 2 discrete run, since the PR lists only the iGPU. Windows 11 26200, AMD Radeon RX 6700 XT (gfx1031), driver 32.0.21045.5002, WinLibs GCC 15.2.0 r7, Release,
AI-assisted: yes (Claude Code). |
Thank you for the test! It covers the gap in my testing and is valuable information. |
|
A new report, #7344, gives a second AMD RDNA 2 result: |
|
I ran it on this head. #6976 does not cover #7344. Windows 11, AMD Radeon(TM) Graphics iGPU (RDNA 2, 1002:164e), WinLibs GCC 15.2.0 r7, Release,
So #7344 reproduces on a second RDNA 2 device and needs a separate fix. I'll keep #6976 scoped to #6666. No open PR claims #7344 yet (#7351 lists it as an outstanding failure), so I'll take it next, since it reproduces here and I already have the ISA dump setup from #6976. AI-assisted: yes (Claude Code). |
|
Fixes it on the iGPU from #6666, which is on a different driver build from the one in the description (32.0.21045.5002 vs 32.0.21042.62). Windows 11 Pro 10.0.26300, MinGW-w64 GCC 15.2.0 (WinLibs r7), Release,
No test fails in B that passes in A on either device. The failures common to both builds are A question on the dropped fences, not a reported failure: the AI-assisted: yes |
|
Thanks for the run, especially on the 21045 driver build. On the fences: no guest pattern depends on them, because the lock already gives the same ordering. AI-assisted: yes (Claude Code). |
jotitadev
left a comment
There was a problem hiding this comment.
Apple GPU run, as a non-AMD check that the exchange release and the dropped in-lock fences still give correct results. On Apple, as on other GPUs, every 64-bit LDS atomic goes through the workgroup lock: MoltenVK reports shaderSharedInt64Atomics = false.
Machine: Apple M2 Max, macOS 27.2, x86-64 build under Rosetta. Head a663dfc. Method: macos-port @ b7eb6b6 + ImportWatched + PR diff, not main. The diff applied cleanly.
- Test only, emitter unchanged:
agc_spirv_lds_atomic_lockfails witha ds_add_u64 is not serialised by one elected lane taking the workgroup lock. That is the message in the description, so the new check runs here. - Full PR, all 13
ldstests pass (ctest -R lds,lds_atomics64excluded) on MoltenVK 1.4.2. That includesagc_spirv_lds_atomic_lock,agc_driver_lds_mixed_width_atomics,agc_driver_lds_integer_atomics,agc_driver_lds_atomicsand both pixel LDS tests. --repeat until-fail:20onagc_driver_lds_mixed_width_atomics,agc_driver_lds_integer_atomicsandagc_driver_lds_atomics: 20/20 on MoltenVK 1.4.2 and 20/20 on MoltenVK main + SPIRV-Cross #2704. No hang and no wrong sum. The mixed-width test uses a 256-thread workgroup, which is 8 subgroups of 32 on this GPU, so the multi-wave case behind #6666 runs here.- Full
^agcsuite with the PR (SDK driver,-j1 --timeout 30, flip/display/lds_atomics64 excluded): 13 failures out of 433, the same 13 as the port baseline. No new failures.
On this port baseline, agc_driver_lds_mixed_width_atomics already passes without the PR, so Apple does not show the RDNA 2 hang. What this run shows is that the new lock form does not break Apple.
GPUs tested: Apple M2 Max (MoltenVK 1.4.2 and MoltenVK main + SPIRV-Cross #2704)
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
@CrimsonMac Independent Windows validation after #7368 merged, on a different AMD iGPU (Ryzen 9 9950X3D, AMD Radeon(TM) Graphics
This supports the combination of #7368 and #6976 for the remaining AMD cases. The misaligned-address fault printed by the lock-budget fixture is its expected negative control. No Linux result, title unblock, or performance improvement is claimed by this run. Your original authorship and commit history are retained in the fork integration. AI-assisted: yes (Codex). |
|
Thanks for the expanded matrix. This adds an independent RDNA 2 discrete GPU, a second AMD iGPU, and an NVIDIA control, and it specifically checks the combined #7368/#6976 path against the two AMD failures. The result supports that tested composition on those Windows drivers; it does not add a Linux or title-run claim. |
a663dfc to
317a323
Compare
|
Rebased onto current |
What
Fixes #6666. On AMD RDNA 2 under Windows, a compute program with 64-bit LDS atomics hangs once its workgroup has more than one wave:
agc_driver_lds_mixed_width_atomicsruns for about 4 s and writes nothing (a TDR on an RX 6400, #5271).The driver's ISA (
VK_KHR_pipeline_executable_properties, Ryzen 5 7600 iGPU, 0x164e, driver 32.0.21042.62) shows why.LockedLdsUpdatereleases the workgroup lock withOpAtomicStore(Release | WorkgroupMemory, Workgroup scope), and the driver compiles that release tos_waitcnt lgkmcnt(0),s_barrier,ds_write_b32. The 32-bit atomics inside the lock add a seconds_barrierthrough theirOpMemoryBarrier(AcquireRelease, Workgroup). The wave holding the lock waits at the barrier, and the other waves never reach it because they loop on the lock. This matches @veosett's capture on an RX 6900 XT in #6670.On the same driver, a release on a read-modify-write atomic does not produce a barrier. The lock is now:
OpAtomicExchangewith the same scope and semantics (Release | LDS memory, Workgroup). The driver compiles it tos_waitcnt lgkmcnt(0)+ds_write_b32, which still waits for every earlier LDS access before freeing the lock.OpMemoryBarrierafter a 32-bit LDS atomic (Atomic32,SharedAtomicUpdate) and at the end ofAtomicUpdateTyped's compare-exchange loop is not emitted between acquire and release. In a program that takes the lock, every LDS atomic runs under it, and the lock's acquire and release use the same scope and storage classes as those fences, so the next holder's acquire already sees everything the previous holder did. Fences outside the lock, and programs without 64-bit LDS atomics, are unchanged.Release forms tried on the RDNA 2 iGPU (
agc_driver_lds_mixed_width_atomics):OpAtomicStoreRelease (main)OpMemoryBarrierRelease + relaxedOpAtomicStoreOpAtomicExchangeReleaseOpAtomicCompareExchangeReleaseOpAtomicExchangeRelease (this PR)OpAtomicCompareExchangeReleaseagc_spirv_lds_atomic_locknow checks that the lock is released by an exchange with Release semantics and that noOpMemoryBarriersits between acquire and release, fords_add_u64,ds_add_u32,ds_inc_u32and a flat atomic into the LDS aperture. Against main's emitter it fails ("a ds_add_u64 is not serialised by one elected lane taking the workgroup lock"); with only theAtomic32fence restored it fails with "ds_add_u32 emits a memory barrier while it holds the workgroup lock".No title is known to hit this. The shader census in #4061 found no 64-bit LDS atomics, so this fixes the hang on RDNA 2 and GCN rather than a game.
#6670 changes the lock's initial store and adds a check to the same test file; the two changes are independent, and whichever merges second needs a small rebase of
SpirvLdsAtomicLock.cpp.Tested
Windows 11 (26100), Ryzen 5 7600, WinLibs GCC 15.2.0 r7, Release,
mainbf2053816plus this commit.agc_driver_lds_mixed_width_atomics, mainlane 0 read 0xdeadbeefdeadbeef--repeat until-fail:20agc_spirv_lds_atomic_lockctest -j 8 --timeout 300agc_driver_layered_volume_vertex_no_gpl, as on main (#5804)agc_driver_f16_fma_denormal_modesandagc_driver_fma_mix_f16_denormal_modes(below)inf * 0(0x7ffffor0xfe00,0x7ffffffffor0xffc00000); the shaders use no LDS, and the f16 one fails the same way on unchanged main (reported on fix(shader): flush v_fma_f16 family denormals by the f16 denormal field #7034).guest_shader_linkno longer hangs at exit after fix(tests): end Windows test processes through libc before DLL detach #6986; the earlierDriver::stop()hangs (Windows: agc_shader_helpers intermittently hangs at exit in Driver::stop() under ctest #6791) are gone from both full runs.python tools/check_conventions.py --base upstream/mainandgit diff --checkpass.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)