Repository navigation
fix(shader): order an image store before a later image atomic on the same texel - #10652
maximilianfv wants to merge 1 commit into
Conversation
…same texel An invocation that stores to a storage image and then reads the texel back with an image atomic got the value from before the store under MoltenVK: Vulkan orders the two by program order, but Metal makes a thread's texture write visible to its own later texture atomic only after a texture fence, and SPIRV-Cross fences texture writes before reads but not before texture atomics. Emit OpMemoryBarrier with device scope and AcquireRelease | ImageMemory before each 32-bit image atomic; MoltenVK turns it into a texture fence, and other drivers already give the ordering. Co-authored-by: mugurc <20699965+mugurc@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 720af6ea444f3e7b7f441946a35db9c844fab919. The targeted MoltenVK report is detailed, but I’m leaving this review commented pending three items. First, the exact barrier line is also present in open #5021; your comment there proposes #10652 first, but I see no agreement yet, and this checklist still says Depends on: none. Please record the agreed landing order in both checklists so the second PR retains only one copy. Second, the PR body says MSL targets older than 3.2 translate this barrier into an illegal threadgroup_barrier inside divergent atomic control flow; please establish the project’s supported MSL floor or make this path valid for supported older targets. Third, this inserts a barrier before every 32-bit image atomic on every backend, and the PR reports no performance measurement. Please bound that cost with a hot-path measurement or narrow the emitted barrier to the backend/conditions that need it. The full GPU matrix was not run here; hosted policy checks pass and product jobs are skipped.
|
An NVIDIA data point, since the barrier is emitted on every driver. Setup: Windows 11 Pro 26200, RTX 4090 (617.42), WinLibs GCC 15.2.0 r7, Release, full build with
AI-assisted: yes (Claude Code) |
|
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.
|
|
Thanks for the RTX 4090 run. It adds useful cross-vendor evidence: all 109 focused tests pass twice with and without the change, and the validation layer reports no errors on either side. That supports the claim that MoltenVK is the only observed behavior change so far. It does not yet bound the cost of emitting a memory barrier before every 32-bit image atomic, since the report explicitly says no performance measurement was made; a macOS-only/backend-specific emission path would also avoid imposing that barrier on Vulkan drivers that already provide the required ordering. The #5021 landing order and the MSL <3.2 compatibility concern are still unresolved, so my review remains commented. |
|
Linux/RADV, first AMD run — the added barrier is a clean no-regression across three AMD architectures. I compared the PR's base
Identical on both sides on every arm: the only AI-assisted: yes. |
What
An invocation that stores to a storage image and then runs a 32-bit image atomic on the same texel got the value from before the store under MoltenVK. This emits
OpMemoryBarrier(Device scope,AcquireRelease | ImageMemory) beforeOpImageTexelPointerin the 32-bit path ofEmitAtomicOp(SpirvImageEmitter.cpp), so it covers every 32-bit image atomic, the compare-exchange loops included.Cause, from a MoltenVK shader dump of the two failing tests: the recompiler emitted
OpImageWrite, thenOpImageTexelPointer+OpAtomicIAdd, with no barrier between them (the onlyOpMemoryBarrier, Device | AcquireRelease | UniformMemory, comes after the atomic). MoltenVK 1.4.2's SPIRV-Cross turned that intoimage.write(...)directly followed byimage.atomic_fetch_add(...)with no fence. SPIRV-Cross'sreadwrite_texture_fencesfences a write before a later read only, not before an atomic, and Metal makes a thread's texture write visible to its own later texture atomic only after a texture fence. So the atomic returned the texel from before the store (the tests'0xdeadbeeffill). SPIRV-Cross writes the new barrier asatomic_thread_fence(mem_flags::mem_texture, ...)only when the target is MSL 3.2 or later (CompilerMSL::emit_barrieremitsatomic_thread_fencewhen the barrier has no execution scope andsupports_msl_version(3, 2)holds, and athreadgroup_barrierotherwise). Upstream issue for the missing fence: KhronosGroup/SPIRV-Cross#2723. The gap this leaves is recorded indocs/dev/debt/shader.md.Vulkan already orders an invocation's store and its later atomic by program order, so other drivers gave the right result before. The barrier is unconditional, so it costs one memory barrier per 32-bit image atomic on every driver; I made no performance measurement.
Limits:
CompilerMSL::emit_barrieremits nothing outside compute, task, mesh and tessellation control shaders, so on MoltenVK a store followed by an atomic on the same texel in a pixel or vertex shader can still return the old value. The fix that covers every stage belongs in SPIRV-Cross (MSL: no texture fence before an image atomic that follows a write to the same read_write texture KhronosGroup/SPIRV-Cross#2723).threadgroup_barrierinside the per-lane divergent control flow thatEmitValueOrZeroIfConditionemits around the atomic, which Metal does not allow. Only macOS 27.0.1 (MoltenVK 1.4.2, LunarG SDK 1.4.363) was tested.Prior art and credit:
Co-authored-by:trailer.#ifdef __APPLE__). None of the PRs feat: macOS host support (Apple Silicon via Rosetta 2, Vulkan on MoltenVK) #8658 was split into (build: compile the shared libc headers, thread priority and three tests on macOS #10362, build: compile FileStream, error categories, thread priority and two tests on macOS #10419) carry it.Tested
Apple M1 Pro, macOS 27.0.1, x86_64 build under Rosetta 2, Vulkan through MoltenVK 1.4.2 from the LunarG Vulkan SDK 1.4.363.
maindoes not build on macOS by itself, so I tested on mugurc's macOS series (ci/macos-series-2, 27e9689) merged withmainat 522e32e, plus local changes identical in both runs: the macOS build parts of #10419, #10540, #10562, #10641 and #10642, and two of my unopened fixes (theposix_spawnpchild inLazyResolvedAbi.cpp, and the image table reuse chain-pool check, which theimagetest filter also covers). Only this change was toggled between the two runs.Both runs:
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ctest --test-dir build -R 'image|atomic|shader_recompiler|spirv|recompiler' -E 'audio_in|voice' -j4 --timeout 300 -V(108 tests).agc_driver_image_cache_bits(image_store dlc: thread 0 is 0xdeadbeef, expected 0x00000000) andagc_driver_image_texel_status(image_store tfe: thread 0 is 0xdeadbeef, expected 0x00000000).agc_driver_image_plane_bias_on_single_level_volume,agc_driver_image_srgb_sample_copy,agc_driver_image_srgb_sample_copy_forced. No new failures.was not found but was requested by env var VK_INSTANCE_LAYERSline. Both logs have 130Validation Errorlines with the same count for every VUID (54imageViewFormatSwizzle-04465, 50imageViewFormatReinterpretation-04466, 15vkCreateDescriptorSetLayout-support-09582, 9imageCreateMaxMipLevels-02251, 1imageView2DOn3DImage-04459, 1pProperties-04451), so the change adds no validation errors.Not tested on any other driver or GPU, and not with any title.
Checklist
main; no other open PR implements the same functions (except feat(aot): consolidate native ARM64 compiler, runtime and consumers #5021, a 324-file consolidation that also carries mugurc's line; see What)NotImplemented_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