Skip to content

fix(shader): read GLC/DLC BDA dwords with device-scope atomic loads - #2259

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
TheAggie:fix/shader-coherent-bda-atomic-loads
Oct 11, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
TheAggie:fix/shader-coherent-bda-atomic-loads

Conversation

@TheAggie

@TheAggie TheAggie commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

What

A GLC/DLC global load is translated as a Volatile load through a physical-storage pointer. Under the GLSL450 memory model, Volatile doesn't make another workgroup's stores visible within the same dispatch. On NVIDIA, a workgroup that spins on a flag written by another workgroup never sees the store.

This is the pattern of decoupled look-back prefix scans. RADV's acceleration-structure build uses it (the radix sort), and vkQuake (PPSA99010) triggers that build with r_rtshadows. Under AnyPS5 the scan spins until the GPU is lost (VK_ERROR_DEVICE_LOST), or spins silently.

When the non-faulting BDA probe gives a coherent dword read a non-null, 4-byte-aligned physical address, SpirvBdaRead.cpp now emits OpAtomicLoad at ScopeDevice with relaxed semantics instead of OpLoad ... Volatile. That covers the wide path in EmitBdaDwordReads and the coherent single-dword helper in DefineBdaDwordReadFunctions. Unaligned addresses, unmapped addresses and reads that span two BDA ranges still take the existing byte path, which reports faults exactly as before. With byte reads forced (no probe), nothing changes. Non-coherent loads are unchanged.

Tests:

  • New tests/execution/CrossWorkgroupFlag.cpp: 25 hand-assembled GFX10 words, two workgroups. Workgroup 0 stores a flag with global_store_dword glc. Workgroup 1 spins on global_load_dword glc dlc and writes 2 if it sees the flag, or 3 after a 1M-iteration bailout, so a failure can't hang the GPU. It runs as wave64 and wave32. A control run with the flag preset by the CPU checks the harness itself.
    • NVIDIA without this change: result 3 (gave up) for both wave sizes. With it: 2.
    • lavapipe passes with and without the change.

This conflicts textually with #1267, which reworks the same read sites; I'll rebase onto whichever lands first.

Tested

Windows 11 Pro (26200), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release, NVIDIA GeForce RTX 5070 Ti (driver 610.88).

  • Full local suite: 508 tests, 506 passed, 1 skipped; guest_sce_net timed out (it times out locally on unrelated branches too, likely my network filtering). It was run from the generated CTestTestfile.cmake commands because antivirus blocks ctest.exe on this machine.
  • tools/check_conventions.py --base origin/main passes.
  • CrossWorkgroupFlag fails on this NVIDIA GPU without the change and passes with it; lavapipe passes either way.
  • vkQuake (PPSA99010) on my local test branch (current main plus my open RADV PRs and local stubs): the GPU hang during RADV's BVH build went away. The build's output is still wrong for a separate reason I haven't found yet, so r_rtshadows doesn't work yet.

Not tested on Linux or macOS.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • AI-assisted: yes

🤖 Generated with Claude Code

@github-actions github-actions Bot added shader core/shader agc libSceAgc* labels Oct 9, 2026
@TheAggie
TheAggie force-pushed the fix/shader-coherent-bda-atomic-loads branch from fc01be3 to abaae2a Compare October 9, 2026 14:22
@joelfernandes23

Copy link
Copy Markdown
Contributor

Tested exact PR head fc01be39fa2a6e1389a3a8f9e892b53f1b408ed2 on AMD Windows, based on main d70b89989473ba1f6ae13e44e079e67f1f8a44b0.

Environment: Windows 11 Pro 10.0.26200, Ryzen 7 7700X, RX 7900 XT (RDNA 3, Vulkan device 0x744c), AMD driver 32.0.31041.1004, Vulkan 1.4.349. WinLibs GCC 15.2.0 UCRT/POSIX/SEH r7, Release, BUILD_TESTING=ON, SPIRV-Tools OFF. ANYPS5_GPU=7900 XT and ANYPS5_REQUIRE_VULKAN=1.

  • Full build and explicit libs target passed.
  • agc_driver_cross_workgroup_flag passed 30/30 consecutive runs with the PR. The verbose run selects the RX 7900 XT; both CPU-preset controls and both cross-workgroup dispatches report flag 1, result 2 in wave64 and wave32.
  • Restored only SpirvBdaRead.cpp to main, retaining the identical regression source, and rebuilt the test and its dependencies. The test also passed 30/30 runs without the production fix. This is an AMD Windows passing control, not a reproduction of the reported NVIDIA failure.
  • Full CTest with the unmodified PR: 504 passed, 3 failed, 1 skipped, 508 total, 36.83 seconds. Two failures are the independently reproduced existing agc_driver_shader_clock (s_memtime goes backwards on AMD's Windows driver too: #1383's narrow-clock check only matches RADV #1615) and agc_driver_vopc_compare_table (agc_driver_vopc_compare_table fails in wave32 for LG compares in the e32 encoding on AMD RDNA 3.5 #825). The third was a 30-second windows_import_modules timeout; an isolated rerun passed in 4.38 seconds. Its cause has not been established. The skip is guest_video_out_wqhd_detection_no_vulkan.

No title test or native-instruction hardware oracle was performed. No modifications to the PR implementation are proposed.

@TheAggie TheAggie changed the title shader: read GLC/DLC BDA dwords with device-scope atomic loads fix(shader): read GLC/DLC BDA dwords with device-scope atomic loads Oct 9, 2026
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 9, 2026
@TheAggie
TheAggie force-pushed the fix/shader-coherent-bda-atomic-loads branch from abaae2a to 8d6f765 Compare October 10, 2026 05:58
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 10, 2026
@adriadam10

Copy link
Copy Markdown
Collaborator

Reviewed the diff: the atomic path is only taken for mapped, 4-byte-aligned addresses (SpirvBdaRead.cpp:176, :254), the byte path and non-coherent loads are unchanged, and the phi/merge structure is correct. Built merged onto main; agc_driver_cross_workgroup_flag passes on RADV.

What we can't confirm here: the test also passes without the change on RADV/lavapipe (and on an RX 7900 XT per @joelfernandes23), so it only fails on NVIDIA, which we can't run.

@oneandonlydean could you run agc_driver_cross_workgroup_flag on the RTX 4090 with and without this change (expected: result 3 without, 2 with)? If it reproduces, this is good to merge.

@TheAggie: this conflicts with #2349 in SpirvBdaRead.cpp and libSceAgcDriver/CMakeLists.txt; whichever lands second will need a rebase.

@Mouhand-Kaddo

Copy link
Copy Markdown
Contributor

Additional platform validation on standalone PR head 8d6f765bd8e0f6abbcbe2f65b610a41ffd981fd7:

Linux: passed — Arch Linux in WSL2 (kernel 6.6.87.2), GCC 16.2.1 20260810; CMake 4.3.1 / Ninja.

Release / BUILD_TESTING=ON: built the selected test targets; 1 CTest cases, 0 failures, 0 CTest skips.

Tests: agc_driver_cross_workgroup_flag.

Reproduce after building the test targets: ctest --test-dir <build> --verbose --no-tests=error --timeout 30 -R "^agc_driver_cross_workgroup_flag$".

Vulkan: Mesa lavapipe 26.2.4 software Vulkan; no native Linux GPU/retail-title validation.

@klosixgaming-arch

Copy link
Copy Markdown
Contributor

Independent validation on real hardware

I built and ran the PR head 8d6f765b on my own box (not the author's):

  • Windows 11 Pro (26200), WinLibs MinGW-w64 GCC 15.2.0 (r7), Release, BUILD_TESTING=ON.
  • GPUs: NVIDIA GeForce RTX 5090, NVIDIA GeForce RTX 4090, AMD Radeon (iGPU, Ryzen 9950X3D). Driver 610.47.
  • I do not have an RTX 5070 Ti, so I can't reproduce the author's exact GPU — the numbers below are from my 5090/4090/iGPU.

Result with the fix (PR head): agc_driver_cross_workgroup_flag_tests PASSES on all three GPUs — result 2 (flag seen) for both wave64 and wave32, and the CPU-preset control run also returns 2.

  • 5090: PASS
  • 4090: PASS
  • AMD Radeon iGPU: PASS — the author only reported NVIDIA + lavapipe; it also passes on this AMD iGPU.

Rigor check (is the test real, or a pass-through?): I reverted only core/shader/recompiler/SpirvBackend/src/SpirvBdaRead.cpp back to the PR base (00112489) — i.e. OpAtomicLoad ScopeDevice → the old OpLoad Volatile / byte path — rebuilt the test, and re-ran on the 5090:

  • control (CPU-preset flag) still result 2 → the harness itself is correct;
  • the cross-workgroup global_load_dword glc dlc spin → result 3 ("a workgroup did not see another workgroup's flag store") on both wave64 and wave32 → FAIL (exit 1).

Restoring the fix puts it back to PASS. So this is a genuine regression test: it fails on base NVIDIA and the atomic-load change is exactly what turns it green — which matches the author's reported "result 3 → result 2" on NVIDIA.

Bottom line: On my 5090/4090/iGPU the PR is green, the new test is a real (non-pass-through) regression test, and it additionally passes on an AMD iGPU beyond what was reported. I did not test Linux/macOS or the 5070 Ti, and I didn't evaluate the noted textual conflict with #1267 (rebase is the author's call).

Written with AI assistance.

@fma-rc

fma-rc commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

Merged onto main 197ffac this no longer builds (Linux, GCC 16.2.1):

CrossWorkgroupFlag.cpp:44: error: no matching function for call to
'GuestAllocations::Mutation::Add(uint8_t*&, const std::size_t&, bool, bool)'

Add takes a fifth argument (bool gpu) since c84694a, merged 2026-10-10; the other execution tests pass true there. GitHub still reports the PR as mergeable because there is no textual conflict.

With Add(block, BlockBytes, true, true, true) applied locally, agc_driver_cross_workgroup_flag, 5 runs each on Linux:

NVIDIA RTX 5080 (615.78.08) RADV 26.1.4, Radeon 610M
with this change 5/5 pass 5/5 pass
SpirvBdaRead.cpp reverted 0/5 pass (wave64: flag 1, result 3) 5/5 pass

@Silverash-i8

Silverash-i8 commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Same as fma-rc's note above, still the case on current main (d31b57a, and 84cc975) on Windows 11 with WinLibs GCC 15.2.0: CrossWorkgroupFlag.cpp:44 needs the fifth gpu argument of GuestAllocations::Mutation::Add (c11a9cb, merged in c84694a). Sorry for the repeat, I missed the earlier comment.

AI-assisted (Claude Code).

TheAggie and others added 2 commits October 11, 2026 02:41
Coherent (GLC/DLC) global loads were emitted as Volatile loads through
physical pointers. Volatile does not make another workgroup's stores
visible within the same dispatch, so a workgroup spinning on a flag
written by another one (decoupled look-back scans, as in RADV's
acceleration-structure build) never saw it on NVIDIA. Aligned coherent
dword reads now use OpAtomicLoad at device scope; the byte path keeps
the old reads for unaligned addresses.

AI-assisted (Claude Code).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@TheAggie
TheAggie force-pushed the fix/shader-coherent-bda-atomic-loads branch from 8d6f765 to eb33094 Compare October 11, 2026 07:47
@TheAggie

Copy link
Copy Markdown
Contributor Author

Thanks @fma-rc and @Silverash-i8, fixed. The branch is rebased onto main 2bb78d2, and eb33094 passes true for the new gpu argument of Mutation::Add in CrossWorkgroupFlag.cpp, as the other execution tests do.

On Windows 11 / RTX 5070 Ti it builds again, and agc_driver_cross_workgroup_flag passes along with the BDA and recompiler tests I reran with it (4 of 4).

AI-assisted (Claude Code).

@adriadam10

Copy link
Copy Markdown
Collaborator

Checked the diff (atomic path only for mapped, 4-byte-aligned addresses; byte path and non-coherent loads unchanged; phi/merge structure correct). Built merged onto main: agc_driver_cross_workgroup_flag and the rest of the Agc/AgcDriver ctest pass on RADV. The test only fails without the change on NVIDIA, which is confirmed independently by @fma-rc (Linux, RTX 5080: 5/5 with the change, 0/5 reverted, result 3) and @klosixgaming-arch (Windows, RTX 5090: fails reverted, passes with the change). Merging.

@adriadam10
adriadam10 merged commit 7028457 into boykopovar:main Oct 11, 2026
14 checks passed
@jotitadev jotitadev mentioned this pull request Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants