Repository navigation
fix(shader): order buffer and image memory at s_barrier - #5117
nikowolf123 wants to merge 1 commit into
Conversation
s_barrier became an OpControlBarrier whose semantics named only Workgroup memory (and Uniform memory when LDS lives in device memory), so a buffer, BDA or storage image store made by one wave before the barrier was not ordered against a load by another wave of the work-group after it. On RDNA the guest makes the barrier a work-group release with s_waitcnt vmcnt(0) & vscnt(0) before it and, in WGP mode, an acquire with buffer_gl0_inv after it (AMDGPUUsage, Memory Model GFX10-GFX11, work-group fence rows). The recompiler translates neither instruction and does not know the mode, so the translated barrier now carries AcquireRelease over Workgroup, Uniform and Image memory at Workgroup scope in every program except vertex-path and hull programs. It orders the accesses the SPIR-V makes Coherent, such as the GLC accesses through the coherent buffer array; plain buffer, BDA and storage image accesses stay private in the Vulkan memory model, which the new technical debt entry records. The new execution test passes dwords between the waves of 128-thread work-groups through a GLC buffer around s_barrier in wave32 and wave64 and checks the barrier semantics in the emitted SPIR-V. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
599e1f8 to
41aa1c9
Compare
There was a problem hiding this comment.
Careful write-up, and I agree with the reading of the memory model. OpControlBarrier with only WorkgroupMemory doesn't order StorageBuffer, PhysicalStorageBuffer or image accesses between invocations. Since s_waitcnt / buffer_gl0_inv translate to nothing, the barrier is the only place left to carry the release/acquire. The ACO detail (a workgroup barrier waits only for the storage classes it names) explains why this shows up on RADV first.
On the cost, which the description leaves unmeasured: this applies to every s_barrier in compute and pixel-path programs, including the many that only exchange through LDS (reductions, prefix sums, tiled blurs), where the guest's own s_waitcnt covers only lgkmcnt. On those, the barrier now also waits for every outstanding VMEM load and store of the wave, including loads issued early to hide latency, which is a common pattern before an LDS barrier. In a tight LDS loop that can remove most of the memory/ALU overlap the shader was scheduled for. Other vendors may do the same, which is why I would measure it on more than RADV.
Two suggestions:
- Measure it. A title with heavy compute (any UE4/UE5 title with GPU culling or post-processing) on RADV and NVIDIA, with GPU time per dispatch (
APS5_PROFILE_GPU[gputime]by program) for the top programs before and after, would show whether the cost is real. - If it is, narrow the extra semantics to programs that can need them. The recompiler knows statically whether a program has any buffer, BDA or image store (
ResourcePlan/ binding usage). A program whose only stores are LDS stores gains nothing fromUniformMemory | ImageMemoryat its barriers, because there is nothing across invocations for them to order. That keeps the fix exact for programs that exchange through memory and costs nothing for the LDS-only ones.
|
Intel run for this PR: Intel UHD Graphics 770 (Raptor Lake iGPU of an i9-13900K), driver 32.0.101.6129, Windows 11, MinGW-w64 GCC 15.2.0 Release, device picked with
AI-assisted (Claude Code). |
…t) (#481) * fix(recompiler): give vertex-path shaders the NGG group info Ported from upstream PR boykopovar#5081 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): put the compute wave index in TG_SIZE[11:6] Ported from upstream PR boykopovar#5214 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): take the 32_AR export alpha from channel 1 as GFX10 does Ported from upstream PR boykopovar#5110 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): order buffer and image memory at s_barrier Ported from upstream PR boykopovar#5117 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): decode GFX10 scratch offsets as one dword Ported from upstream PR boykopovar#5426 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): accept MIMG sampler tuples ending in VCC Ported from upstream PR boykopovar#4996 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(recompiler): name the attribute of an out-of-range interpolation reference Ported from upstream PR boykopovar#5450 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): decode the high-half result of v_interp_p2_f16 Ported from upstream PR boykopovar#5471. The Vop3InterpDecode unit-test hunk is not ported (that test file is not on main); the execution test InterpolationF16 carries the coverage. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): clear the bits of inactive lanes in VALU carry-out and v_div_scale masks Ported from upstream PR boykopovar#5162 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): apply the f32 output modifier and denormal flush to v_cvt_f32_f64 Ported from upstream PR boykopovar#5159 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): update per-invocation LDS without atomics Ported from upstream PR boykopovar#5160. Conflicts resolved against main: kept main's barrier condition for non-LDS shared atomics; the unit-test hunk for SRT read patching (a different upstream change, not on main) is dropped. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): flush ds_add_f32 denormals and NaNs as the hardware does Ported from upstream PR boykopovar#5492 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): run DS src2 lanes one at a time in lane order Ported from upstream PR boykopovar#5498 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(agc): compare NOT_EQUAL unordered, reject TRUNC_COORD point sampling and round the array layer half to even in color compare emulation Ported from upstream PR boykopovar#907 (follow-up to its predecessor). Hand-applied to main's reworked emitter: NOT_EQUAL uses OpFUnordNotEqual, the array layer is clamp(roundEven(z)), and ResourceMaterializer rejects TRUNC_COORD with point filtering. The upstream execution-test rewrite is not ported (it targets a diverged test file). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * chore(shader): bump ShaderDiskCache FormatVersion to 35 The ported DS fixes add MemoryInfo::flushDenormalResults (now encoded in the plan) and new IR opcodes, so cached payloads from format 34 must not load. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): run ported upstream execution tests on Metal through a device shim The upstream execution tests for the ported fixes compile unchanged against a Metal stand-in for the Vulkan test device (guest buffer descriptors are mirrored into shared MTLBuffers). Covers boykopovar#5162 (VALU carry-out masks), boykopovar#5159 (v_cvt_f32_f64 result modes), boykopovar#5492 (ds_add_f32) and boykopovar#5498 (DS src2 lane order). The LdsSrc2 and LdsSrc2Wide tests use device-memory LDS, which the Metal bridge rejects, and are not registered. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): run ported decoder and collector unit tests in the Metal build Registers the upstream tests for boykopovar#4996, boykopovar#5426 and boykopovar#5450 (already on main's tree but not built for Metal) and a high-half decode check for boykopovar#5471. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): check per-invocation LDS updates without atomics on the Metal recompiler Standalone copy of the boykopovar#5160 verifyFunctionLdsUpdates check. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): check the TG_SIZE wave index on Metal Buffer-store variant of the boykopovar#5214 execution test. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): run the s_barrier memory-order execution test on Metal Registers boykopovar#5117's upstream test through the device shim. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): draw a 32_AR export on Metal Guest-replay draw for boykopovar#5110: alpha comes from export channel 1. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): draw an NGG vertex shader that reads the group info Guest-replay draw for boykopovar#5081. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): reject TRUNC_COORD point sampling in the color compare emulation Recompile-level check for boykopovar#907. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): run the src2 lane-order test without the Metal debug layer Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * docs(shader): port upstream debt entries and add the FormatVersion 35 history line Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Closing: no title is known to reach this. With the queue at 700+ open PRs, please hold off on new PRs that don't name a title needing the change until it's down. If a title stops on this on current main, reopen with the title ID and the log line. |
What
Outside vertex-path and hull programs,
s_barrierbecameOpControlBarrier(Workgroup, Workgroup, AcquireRelease | WorkgroupMemory): semantics0x108, or0x148when LDS lives in device memory. A SPIR-V barrier orders only the storage classes its semantics name. The SPIR-V specification, 3.2.24 Memory Semantics, says: "Storage classes not selected are not being constrained."UniformMemory(0x40) coversStorageBufferandPhysicalStorageBuffer, andImageMemory(0x800) covers image contents. The guest's V# buffers, GPU-selected (BDA) buffers, GDS and storage images all live there. So a store that one wave made before the barrier was not ordered against a load that another wave of the work-group made after it. The Vulkan memory model (Memory Model appendix, inter-thread-happens-before) orders accesses through a release or acquire barrier only when the barrier's semantics include their storage classes. Without that, such an exchange is a data race and the load may return the old value.How the guest gets this ordering on the console:
S_BARRIERmakes each wave wait until the other waves of the work-group reach it.S_WAITCNTso that earlier memory operations have completed.buffer_gl0_inv.s_waitcnt lgkmcnt(0) & vmcnt(0) & vscnt(0)for release, and the same followed bybuffer_gl0_invfor acquire.vmcnt(0)/vscnt(0)and thebuffer_gl0_inv. The release keeps its waits in both modes.s_waitcnt,s_waitcnt_vscntandbuffer_gl0_invto nothing (EmitWaitcntandEmitControlNopinSpirvFlowEmitter.cpp). It also does not know the CU or WGP mode. So the translated barrier is the only place left to carry the work-group release and acquire.On RADV this has an effect. In Mesa ACO,
setup_barrierandfinish_barrier_internal(src/amd/compiler/aco_insert_waitcnt.cppat Mesa9b9e14ed5c99) make a workgroup barrier wait only for outstanding accesses in the storage classes named in its semantics, and only in WGP mode. WithWorkgroupMemoryalone, the barrier does not wait for a pending buffer store. RADV compiles a compute program in WGP mode on gfx10 and later only when it uses no LDS (radv_should_use_wgp_modeinsrc/amd/vulkan/radv_shader.c, same commit); a program with LDS runs in CU mode, where ACO drops these waits at a workgroup barrier.Change:
EmitBarrieraddsUniformMemory | ImageMemory. The semantics become0x948(AcquireRelease | UniformMemory | WorkgroupMemory | ImageMemory), still with Workgroup execution and memory scope.OpMemoryBarrierafter atomics.vmcnt(0)/vscnt(0)wait.s_waitcntreaches the IR as an operand-lessWaitcnt, so the barrier cannot tell these cases apart. Those barriers get an ordering the console does not guarantee. That is allowed, but it may cost host waits (below).lgkmcnt. On RADV this concerns only programs without LDS, the ones it runs in WGP mode.Still open, recorded in
docs/dev/debt/shader.mdnext to the vertex-paths_barrierentry:CoherentnorVolatilein the SPIR-V:buffersarray when it is notCoherent;buffer_gl0_invand has no GLC or DLC access. Making those accesses coherent is a separate change with its own cost (perf(shader): load from buffers without Coherent when the load has no GLC or DLC #2369 measured whatCoherentcosts on NVIDIA).Test:
agc_driver_barrier_global_memory(tests/execution/BarrierGlobalMemory.cpp).0xdeadbeef.llvm-mc21,-mcpu=gfx1030:tidof groupgmust readg * 512 + 127 - tid, the value that thread127 - tidstored.glc=1loads.Coherentbuffer view.OpControlBarriermust have Workgroup execution and memory scope, and semantics that contain0x948.Tested
Not measured on AMD hardware. Everything below ran on lavapipe, except the last run in the list, on an NVIDIA RTX 5080. Reviewers with a gfx103x GPU, please confirm:
tools/hw-oracle, in work-groups of 128 lanes, wave32 and wave64, the hardware reads127 - tid.agc_driver_barrier_global_memorypasses on RADV. If you can, also run it with the change reverted. The test program declares no workgroup memory, so RADV runs it in WGP mode, where that build may read0xdeadbeef; on lavapipe it does not.Setup:
mainat c8eb1f5.cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=ON -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DANYPS5_ENABLE_SPIRV_TOOLS=ON, thencmake --build build -j6.VK_ICD_FILENAMES=/usr/share/vulkan/icd.d/lvp_icd.json ANYPS5_REQUIRE_VULKAN=1 GALLIUM_OVERRIDE_CPU_CAPS=avx.ANYPS5_ENABLE_SPIRV_TOOLS=ON, SPIRV-Tools validates every recompiled module for the target's Vulkan environment, including the new barrier semantics.Results:
mainplus the test only (ctest -R agc_driver_barrier_global_memory --output-on-failure): fails withwave32 s_barrier semantics 0x00000108 do not order buffer, workgroup and image memory with acquire-release (0x00000948). It gets there after the 4 runtime rounds have passed.OpControlBarrierfromEmitBarrier. The runtime part then fails on lavapipe withwave32 round 0 group 0 thread 0 read 0xdeadbeef after s_barrier, expected 0x0000007f. So the runtime exchange does depend on the barrier.ctest -R 'agc_shader|agc_driver' -j6 --timeout 120:main: 377 tests, 341 passed, 36 skipped;ctest -j6 --timeout 120on this branch: 1044 tests, 0 failed, 43 skipped.main14036cb (the branch now sits on 0f2683c with the same three-file change, no conflicts). Native Windows 11, WinLibs GCC 15.2.0, NVIDIA GeForce RTX 5080 (driver 616.92): the new test passes 3 of 3 runs, andctest -R 'agc_shader|agc_driver|shader|recompiler'passes 383 of 383.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