Repository navigation
fix(shader): use 64-bit buffer atomics for device-memory LDS - #7368
oneandonlydean merged 2 commits into
Conversation
Compute LDS beyond the device's shared-memory limit lives in a storage buffer, and its 64-bit atomics took a lock in that buffer. AMD's Windows driver compiles the lock's acquire and release to s_barrier, which inside the divergent lock loop pairs with the program's own barriers, so the device-memory case of agc_driver_lds_lock_budget read half its sum. With shaderBufferInt64Atomics the device-memory LDS now uses a 64-bit view of the buffer instead: 64-bit atomics are compare-exchange loops on it and 32-bit atomics stay native, so no lock is taken. Without the feature the lock is kept. The per-workgroup slice is rounded up to an even dword count so that the 64-bit view stays aligned.
|
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.
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I noticed the new device-memory LDS path guards a 64-bit atomic whose address is 4-byte but not 8-byte aligned by returning zero. The existing LDS atomic debt entry says this case faults the wave on gfx1036. That makes the newly selected path silently succeed where the measured hardware faults. Could you add fault/refusal handling and regression coverage for this case, or explain why returning zero is acceptable for device-memory LDS?
|
Good catch. Returning zero wasn't defensible, since gfx1036 faults the wave there. Coverage: I kept the workgroup-lock path as it is (it still does the split update, and the debt entry already says the fault isn't modelled there). Making it fault too would be a separate change. Tested on Windows 11: RTX 4070 SUPER full ctest 1443/1443. Radeon iGPU (gfx1036, 32.0.21042.62) 1440/1443: #5804, plus AI-assisted: yes |
|
Linux comparison on exact PR head
Each PR run verified GPU readback across all 256 lanes for both On this PR, This supplies the missing Linux/lavapipe comparison: the Windows RDNA 2 failure in #7344 does not reproduce on this RADV gfx1036 device at the pinned main above, and this PR retains passing results here. The devices expose different shared-memory limits, as listed. No claim about Windows, a physical device without buffer int64 atomics, title behavior, or a full-suite run. Khronos validation layers are unavailable on this host; these are readback/fault-check results, not validation-layer coverage. |
…ot fault The last sentence of the 64-bit LDS atomics entry said that on the device-memory path with shaderBufferInt64Atomics a 64-bit atomic at an address that is 4-byte but not 8-byte aligned is skipped and returns 0. cb28abe (boykopovar#7368) made that path record the Unaligned shader fault, which the entry now states, so the sentence described a gap that is closed. The gap that is still open is on every other path: SharedAtomic64 checks the alignment only on that path, so with the workgroup lock (in shared or in device memory) and in the invocation-private LDS of the other stages a ds_* 64-bit atomic at such an address updates the two dwords there, where gfx1036 and gfx1035 fault the wave. Co-Authored-By: Claude <noreply@anthropic.com>
…ot fault The last sentence of the 64-bit LDS atomics entry said that on the device-memory path with shaderBufferInt64Atomics a 64-bit atomic at an address that is 4-byte but not 8-byte aligned is skipped and returns 0. cb28abe (boykopovar#7368) made that path record the Unaligned shader fault, which the entry now states, so the sentence described a gap that is closed. The gap that is still open is on every other path: SharedAtomic64 checks the alignment only on that path, so with the workgroup lock (in shared or in device memory) and in the invocation-private LDS of the other stages a ds_* 64-bit atomic at such an address updates the two dwords there, where gfx1036 and gfx1035 fault the wave. Co-Authored-By: Claude <noreply@anthropic.com>
What
Fixes #7344. When a compute program's LDS does not fit the device's shared-memory limit, the LDS lives in a storage buffer (
workgroup_memory), and a program with 64-bit LDS atomics takes the workgroup lock in that buffer. On AMD's Windows driver the device-memory case ofagc_driver_lds_lock_budgetreads exactly half its sum (0x00000ffffffff800for0x00001ffffffff000), on @Amoenus's iGPU and on mine.The driver's ISA (
VK_KHR_pipeline_executable_properties, Ryzen 5 7600 iGPU, 0x164e, driver 32.0.21042.62) shows the cause. Every acquire or release on the storage buffer becomes a workgroup barrier:The pipeline has 83
s_barriers; the program needs 3 (its own two and one after the lock is initialized). Each wave runs the lock loop a different number of times, so these barriers pair with the program's owns_barrierbefore its finalds_read_b64, and lane 0 reads before the other waves have finished their atomics. Unlike #6666 this does not hang, and #6976 does not fix it (checked on #6976): its exchange release and the fences it drops are not the only barriers here.What I tried on the iGPU, changing only the lock (all rows but the first with #6976 applied, so that the fences inside the lock are gone):
s_barrierss_waitcnt_vscntbefore the release, so the next holder can read before the stores landNo form of the buffer lock is both ordered and free of barriers on this driver. The data is already in a storage buffer, which can take native 64-bit atomics, so this PR stops using the lock there:
shaderBufferInt64Atomics(theInt64Atomicscapability in the target), a program whose LDS is in device memory no longer requests the lock. A 64-bit LDS atomic (ds_*_u64/_b64/_f64, and a 64-bit flat atomic into the LDS aperture) is a compare-exchange loop (AtomicUpdateTyped) on a 64-bit view of the same binding (workgroup_memory_u64, Aliased and Coherent like thebuffers_u64view of guest buffers). The 32-bit LDS atomics stay native buffer atomics, as in programs without 64-bit atomics.LDS bytes / 4 + 1dwords rounded up to an even count, so the 64-bit view stays 8-byte aligned in every workgroup. The extra dword is still the lock without the feature.ds_*64-bit atomic whose address is 4-byte but not 8-byte aligned (it faults the wave on gfx1035/gfx1036, pershader.md) is skipped there and returns 0; the flat form already records an unaligned fault and skips.The fence that follows every LDS atomic (
Atomic32, and the end ofAtomicUpdateTyped) is still emitted, and on this driver it is ans_barriertoo. It runs once per atomic outside any loop, so waves that all execute the atomic pass it the same number of times, as for 32-bit LDS atomics in every program today; a wave that skips an atomic other waves execute would miss one. That is unchanged by this PR and recorded inshader.md.Mixed widths on one slot now rely on the device ordering a 32-bit and a 64-bit atomic on one buffer address.
ds_add_u32againstds_add_u64sums exactly on both GPUs below. The other pairs were not measured; this is recorded indocs/dev/debt/shader.md, and the slice layout indocs/dev/debt/libSceAgcDriver.md.agc_lds_lock_budgetnow also compiles each case withInt64Atomics. It checks that device-memory LDS then uses a 64-bitOpAtomicCompareExchangeand no 32-bit lock, and that in-budget LDS, or a target without the capability, keeps the lock. The stride expectations in it,agc_driver_lds_beyond_device_limitandagc_driver_lds_lock_budgetmove to the even count.On AMD the second case of
agc_driver_lds_lock_budget(limit-4, LDS in Workgroup memory) still fails with this PR alone, because it is the workgroup-lock hang of #6666, fixed by #6976. The two PRs touch different code and merge cleanly together; with both, every LDS test passes on AMD.Tested
Windows 11 (26100), Ryzen 5 7600, WinLibs GCC 15.2.0 r7, Release,
ANYPS5_REQUIRE_VULKAN=1. The rows below are onmainb39d90bf1(which has #7351, sorobustBufferAccess2=0on the AMD driver; the case fails the same with it on, as in #7344) plus this commit, except the full runs, which were on195d0d08aplus this commit.agc_driver_lds_lock_budget, mainlimit:lane 0 read 0x00000ffffffff800limitpasses (device=8194);limit-4fails as on main (#6666)--repeat until-fail:20agc_lds_lock_budget,agc_spirv_lds_atomic_lock,agc_driver_lds_beyond_device_limit,agc_driver_lds_mixed_width_atomics, this PR merged with #6976,--repeat until-fail:20ctest --parallel 4 --timeout 300, this PR on195d0d08aagc_driver_lds_mixed_width_atomicsand thelimit-4case ofagc_driver_lds_lock_budget(#6666), andagc_driver_subvector_loop,agc_driver_layered_volume_draw,agc_driver_layered_volume_vertex(fail on thatmaintoo, fixed by #7351; they pass onb39d90bf1with this PR)0xc0000142at the end of the run while the machine ran out of memory; all 11 pass when rerun.agc_driver_oversized_dispatch, which also takes the device-memory path, skips on the iGPU and passes on NVIDIA.agc_driver_lds_mixed_width_atomics_testspassesspirv-val --target-env vulkan1.3(SPIRV-Tools from3rdparty).shaderBufferInt64Atomics.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)limit-4case on AMD, not for this change)