Repository navigation
fix(shader): keep faulting invocations in their wave and mask their later writes - #6416
Celegans12 wants to merge 3 commits into
Conversation
|
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.
|
|
@Celegans12 a data point from Ghost of Yōtei (PPSA26344): RTX 4090, driver 617.42, Windows 11. This PR on main 0dab493 + #5815 #5781 #6227 #5405 #6402 #6174, with In our base, this PR conflicted in It still loses the device, at ~1,460 s (frame ~817):
That's the same signature as our earlier spanned runs without this PR. The non-spanned build of 0x80001e5f00 (839,036 words) dispatched fine. The spanned pipeline (838,306 words) was created about 5 minutes before the loss, but the log has no dispatch line for it. After it came several minutes of cold compiles of new 0x801d* programs, and the loss hit while 0x801d29eb00 was being created. So I can't tell whether that program or another spanned one hung. With spanned waves off, the same base runs well past that point. If you have a suspect, I can rerun with more logging. |
|
A cost I measured tonight that reviewers should know about: on NVIDIA, the write mask roughly doubles pipeline compile time for PPSA26344's giant two-lane compute programs. Compiled side by side on the same machine, compute 0x801cdd2700 (about 1.75M SPIR-V words) takes 700 s with the earlier "reads continue, nothing masked" version and 1,411 s with this PR. The module is only about 0.5% bigger: roughly 90 more branches and about 550 more stores and loads of the per-lane faulted flag. So the extra time looks like NVIDIA's compiler handling a flag that stays live across the whole program, not the extra code itself. For now our game branch keeps the older version, so our precompiles don't take twice as long. I'll look at making the mask cheaper, for example only guarding writes that a fault can reach, or keeping the flag in a form the compiler handles better, and post numbers here. If anyone has a preferred shape for this, I'm happy to hear it before I start. Details
|
|
Tested on Linux with lavapipe, the CI device: llvmpipe (Mesa 26.2.4, LLVM 23.1.1), CachyOS, GCC 16.2.1, CMake 4.4.3, default build type, full build with This PR (
The reason, from the code: lavapipe reports 8-wide subgroups ( if (device->Target().subgroupSize != 32u) {
std::printf("skipped, the device runs wave64 programs on %u-wide subgroups\n", device->Target().subgroupSize);
return VulkanTestSkipped;
}would make lavapipe report a device-limited skip ( This may be useful for the mask rework you mention: on lavapipe the test passes even with the fix reverted, so it can't show whether a cheaper mask still handles the two-lane cases. That can only be checked on a 32-wide device, such as your RTX 3080 Laptop GPU. Merged together with #6402 (on
Apart from those, the failed and skipped lists match Not tested: NVIDIA, AMD, Intel, PPSA26344. AI-assisted: yes |
|
Barrier-program follow-up: #6689. I reproduced later guest writes after an unmapped BDA read plus Proposed follow-up reuses your per-lane mask, keeps the invocations at their barriers, and carries the checked BDA helper's fault result back to the correct lane half. All-fault and mixed valid/faulting lane tests pass 5/5 per GPU. The full Windows build and focused checks passed, but broader regression/static-analysis gates remain incomplete; the current checkpoint and remaining checks are recorded in #6689. This prototype is deferred while we pivot to PR maintenance. Proposed ordering is #6416 first, then this compute follow-up. If you prefer to incorporate it here, the focused commit is available to take. No competing upstream PR is open. AI-assisted: yes. |
|
While preparing the barrier follow-up, I checked the narrow-subgroup case raised in the review. The current |
|
I opened stacked follow-up #6751 for the test-only subgroup guard discussed here. The |
|
Rebase handoff — rebased onto current Verified on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ninja, Release,
To pick it up: Head is |
|
Thanks. Main has moved under this PR: since bd6ed97, |
|
Correction to my earlier note: spanned waves are opt-in on |
jotitadev
left a comment
There was a problem hiding this comment.
Apple GPU / MoltenVK run. Apple's subgroup size is exactly 32, the case agc_driver_faulting_wave_invocation needs (see #6751): red without the fix, green with it, and no other change.
Machine: Apple M2 Max, macOS 27.2, x86-64 build under Rosetta. Head 9c76817, PR diff applied cleanly. Driver: MoltenVK 1.4.2 (Vulkan SDK 1.4.363.0), subgroup=32.
Tested: the tree is main a373e85 + mugurc's macOS series (ci/macos-series-2) + the PRs needed to build on macOS (#6667, #6668, #7524, #6969, and two files of #7332), not plain main (it doesn't build on macOS yet). Base on that tree: 40 ^agc failures, the same known failures in every run below unless noted.
agc_driver_faulting_wave_invocationwith the PR: passes 3 of 3.- The test alone on the base tree: fails with
lane 0 left the EXEC loop after 100 passes, expected 4 (the loop stops at 100). - Full
^agcsuite with the PR: 40 of 728 fail, the same 40 as the base.
About the spanned-wave gap @oneandonlydean described: it can't occur on MoltenVK today. With APS5_SPANNED_WAVE=1 the driver logs spans two full 32-lane subgroups: 0, because MoltenVK doesn't give full compute subgroups, so wave64 always takes the two-lane path here and the masking stays on.
GPUs tested: Apple M2 Max (MoltenVK 1.4.2)
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
@oneandonlydean, thanks for doing the rebase and for sorting out the spanned-wave question. Yes, I'll take the spanned-wave masking as a follow-up PR once this one is in. The plan is what you describe: record the fault in About Yōtei: our runs do use |
…x for named wave-operation programs (boykopovar#6416 masks elsewhere)
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (2, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 390 tests)Skipped (41)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
stale+rebase 1. Freshness Envelope
2. Findings
3. Cross-PR Ledger
|
9c76817 to
8e6e734
Compare
|
I've rebased this onto main e2df06e as one conventional commit, preserving main's spanned-wave exclusion and mesh-window initialization. The fault-mask patch is otherwise unchanged, and the missing All four focused tests pass on the RTX 3080 Laptop GPU. With main's runtime code and only the new fixture added, the three existing controls pass but the fixture fails because partner lane 37 stays untouched ( The Astro startup delay is still open: masking adds a flag load and branch around qualifying writes even without faults, so a warm cache and no pipeline misses don't rule out shader execution cost. The missing texture-budget message is conditional reporting and doesn't establish skipped budget work. I haven't run titles on this rebased head; paired main/slow/fast startup logs with the exact settings and revisions, or a reduced reproduction, are still needed to explain the delay. |
❌ NVIDIA GPU test: failedPR head
Failing testsThis PR changes test files, so each failing test was also built and run on main with only the PR's test changes applied. ✅ there means the failure comes from the PR's other changes; ❌ means the test fails on main too, so this run does not show the PR caused it.
agc_driver_guarded_null_pointer: Failed (also failed when re-run alone), assertion and diagnosticsUnstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (5 IDs in 480 tests)Skipped (44)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Hey @Celegans12, I checked the rebased Windows 11 Pro 26300, WinLibs GCC 15.2.0 r7, Release, bundled SPIRV-Tools:
Both GPUs report native subgroup 32. Intel reports With main's runtime and only your exact new fixture added, For #7392 I applied only its barrier-write and test-gating commits over the updated prerequisite, preserving authorship and main's mesh initialization. No second copy of the old prerequisite. The same helper assertion and direct/draw span-report failures remain until the two supplied test corrections are applied. Those three corrected tests also pass three fresh-process runs per GPU, 18 additional executions. Turning only All seven source arms passed the full default Windows build (FFmpeg reused its pinned prebuilt cache). Focused runs required Vulkan, disabled the application shader cache, excluded implicit layers and actually loaded Khronos validation. No VUID/SYNC errors or test timeouts were observed. Stale installed-layer manifest lookups still emit loader discovery errors. The full CTest suite was not run. The refreshed test-only commits are This is focused correctness evidence, not an Astro startup, title-unblock or performance result. AMD/native64, Linux and console behavior were not tested here. AI-assisted, with independent source and raw-evidence review. AnyPS5-6416-7392-verified-stack-20261031.zip ZIP SHA256: |
…ater writes Keep faulted wave participants voting instead of returning, while masking their later writes, atomics and exports. This also preserves the healthy partner of a two-lane invocation. Retain existing eligibility exclusions. Latch the first guarded SRT fault so later words cannot replace it. Strengthen host assertions; retain the runtime address and PC oracle. Based on ThatsLeo/AnyPS5@b9362d8 core/shader/recompiler/SpirvBackend/src/SpirvMemory/SpirvMemoryInstructions.cpp (author: ThatsLeo). Adapt the actual-stopping predicate for masked lanes. No spanned-wave policy from boykopovar#7922 is included. AI-assisted: yes
8e6e734 to
4414d78
Compare
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (5 IDs in 481 tests)Skipped (45)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Keep spanning wave invocations active for subgroup operations while masking writes after a fault. Guard byte loads when fault handling records a lane fault without returning from the invocation.
|
@cookie42073-cmd @TechLevelZero I merged current main and found that the faulting-wave test failed when main selected a hardware-spanning wave: the faulted lane continued and performed its later store. The follow-up keeps those lanes participating while masking their writes, and guards the byte-load path when recording a fault doesn't return from the invocation. With shader caching disabled and Vulkan validation enabled, the wave case, guarded-pointer cases, guarded-load checks, wave decoding and shader-cache test each passed twice on Windows with an NVIDIA RTX 3080 Laptop GPU. The updated emitter, BDA read and shader-cache sources also compile on Linux with the layout guards enabled. Could you rerun the updated head when convenient? |
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 512 tests)Skipped (44)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — approved
The merge with main is clean and builds. The focused tests pass with no new validation IDs, and the updated guarded-pointer host check fails without the production change. This machine's lavapipe cannot run the wave64 two-lane path or the spanned-wave path, so for those paths I rely on the NVIDIA runs that other people reported.
What I ran
Head 19eddecba1143143bb4c5b750f51632a4cf2eef8, main 9090972b7e91fd266819cbd77a8b058e177b5682, merged tree 8d69bdea97d7ba1a41e27e654c4ef880fb16bf9c (--no-ff). Machine: Debian 13 VM, 2× Xeon Gold 6136, GCC 14.2.0, Release, Ninja, lavapipe (Mesa 26.1.6, LLVM 19, subgroup 8).
git diff --check 9090972b7e91fd266819cbd77a8b058e177b5682...19eddecba1143143bb4c5b750f51632a4cf2eef8
python3 tools/check_conventions.py --base 9090972b7e91fd266819cbd77a8b058e177b5682
env ANYPS5_REQUIRE_VULKAN=1 GALLIUM_OVERRIDE_CPU_CAPS=avx ctest --test-dir build -j8 --test-load 32 --output-on-failure \
-R "^(agc_driver_faulting_wave_invocation|agc_driver_guarded_null_pointer|agc_shader_memory|agc_driver_bda_device|agc_driver_bda_span_reads|agc_driver_bda_span_reads_draw_pipeline|agc_driver_bda_anchor_reads|agc_driver_gpu_buffer_format)$"
env ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 build/tests/agc_driver_faulting_wave_invocation_tests
# negative control: the PR's 8 production files restored from main, then
ninja -C build -j32 -l32 agc_shader_memory_tests agc_driver_guarded_null_pointer_tests agc_driver_faulting_wave_invocation_tests
ctest --test-dir build --output-on-failure -R "^(agc_driver_faulting_wave_invocation|agc_driver_guarded_null_pointer|agc_shader_memory)$"
# merged files restored from 8d69bdea, rebuilt, same ctest again; git status --porcelain empty
Results
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
| Configure / build / libs (script) | ok |
| Selected tests (script) | 2033 ran: 0 new failures, 0 timeouts. 7 known main failures (agc_driver_division, denormal-mode tests, unwrap_self, vector_constant_factor_tininess). agc_driver_runtime_formatted_store passed on rerun |
| Skips | 111, all with device-limitation reasons (check_skips.py) |
Vulkan validation vs main (script) |
758 tests, layer loaded in 757, missing in none, 0 tests with new VUIDs |
| Focused tests (8) | all pass. agc_shader_memory 8.7 s, every other test under 3.4 s |
| New test registration | picked up automatically by the tests/execution/*.cpp glob as agc_driver_faulting_wave_invocation |
New test on lavapipe
Selected GPU name=llvmpipe (LLVM 19.1.7, 256 bits) ... subgroup=8
single-wave wave64 compute spans two full 32-lane subgroups: 0
[gpu] deferred write-back failed: BDA access failed: address=0x... instruction=0x10 bytes=1 stage=4 reason=1
faulting wave invocation tests passed
Negative control
I restored the 8 production files from main and kept the PR's tests: ShaderDiskCache.hpp, SpirvBda.hpp, SpirvEmitterState.hpp, SpirvBdaFault.cpp, SpirvBdaRead.cpp, SpirvEmitter.cpp, SpirvFlowEmitter.cpp and SpirvMemoryInstructions.cpp. Then I rebuilt the three affected test executables.
agc_shader_memoryfails as it should:guarded pointer: a continuing guarded read must latch its first fault without overwriting the guard word or calling record_bda_fault.agc_driver_faulting_wave_invocationstill passes. That is expected on this device: with 8-wide subgroups,laneCountis 1 and no wave spans subgroups, so neither the two-lane path nor the spanned path is reached. This confirms with my own run what Dextroid17 and MarcZX10 reported for lavapipe. The red-without/green-with evidence for this test comes from jotitadev (MoltenVK, subgroup 32), from cookie42073-cmd (RTX 4070 SUPER, UHD 770) and from the author.agc_driver_guarded_null_pointerpasses in both arms on lavapipe.
After restoring the merged files and rebuilding, all three tests pass again and git status --porcelain is empty.
Findings
Blocking
None.
Non-blocking
- The description no longer matches the last commit. The description says "Existing spanned-wave, barrier and mesh exclusions remain". But
19eddecba1removes!state.spannedWavefrombdaStopsInvocations(core/shader/recompiler/SpirvBackend/src/SpirvEmitter.cpp:331). It then enables write masking for spanned waves through(waveOperations || state.spannedWave)(SpirvEmitter.cpp:335). Spanned waves are on by default onmainfor devices with full 32-lane compute subgroups: they are disabled only byAPS5_NO_SPANNED_WAVE, inVulkanDevice.cpp:2098. So on NVIDIA this is now the main path that the new test exercises. This is in effect the spanned-wave follow-up that oneandonlydean asked for. Please update the "What" paragraph so reviewers know that spanned compute waves now continue with masked writes instead of being excluded. Pipodi's PPSA26344 report of a device loss with spanned waves plus masking was made on an earlier version, so a rerun with this head would be useful. - The technical-debt entry for unlinked calls is now partly outdated.
docs/dev/debt/shader.md:47says an unlinked call "records theUnlinkedCallfault ... and stops the invocation, or continues after the call in programs whose faults do not stop invocations". With this PR,EmitUnlinkedCall(SpirvFlowEmitter.cpp:959) →StopBdaInvocationIf(SpirvBdaFault.cpp:132) no longer stops in masked programs. The invocation instead continues past the call with its later guest writes masked. A one-clause update of that entry would keep the debt file accurate. - Lavapipe CI cannot detect a regression in the new test. I confirmed this with the negative control above; Dextroid17 and MarcZX10 had already reported it. #6751 adds the subgroup-32 skip. Mentioning that dependency here, or folding the guard in, would avoid a test that passes on CI without exercising anything.
- Cost without faults. The author reports roughly 2× NVIDIA pipeline compile time on very large two-lane programs, and an Astro Bot startup delay on an RTX 4090 is still unexplained. I did not measure either. The PR makes no performance claim, so this is not a rule issue, but it is worth tracking before titles with large DMA compute programs depend on it.
Not verified
- Behaviour on a device with 32-wide subgroups: the two-lane wave64 path and the hardware-spanning path. This machine only has lavapipe with subgroup 8, so these come from third-party runs (TechLevelZero's RTX 2000 Ada run on this exact head, the author's RTX 3080 Laptop GPU run, and earlier MoltenVK, RTX 4070 SUPER and UHD 770 runs).
- Whether every
StopBdaInvocationIfcaller that may now continue in a masked program guards its later accesses. The byte-load path is guarded inSpirvBdaRead.cpp:44-45. I read the runtime-buffer and overflow callers, and they look guarded or zero-valued, but I did not check all of them exhaustively. - Compile time and run time impact, and title behaviour (PPSA26344).
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (11, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 510 tests)Skipped (53)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Hey, I reran the spanning-wave follow-up on Intel UHD 770 (32.0.101.7088) and RTX 4070 SUPER (621.01), Windows 11 Pro 26300, GCC 15.2.0 Release with SPIRV-Tools enabled. The new four-line change fixes the reproduced failure on both GPUs. I used pinned main
The fixture differs from yours only by one metadata print after recompilation. Its guest program, faulting lane and assertions are unchanged. The emitted result reports Reverting only the four-line follow-up brings the spanning failure back on both GPUs, while all nine companion tests still pass. After restoring it and completing the full build, the ten-test matrix and the separate nonspanning run pass on each GPU: 22 final checks. The matrix covers the faulting wave, BDA span/draw-pipeline reads, guarded null/direct-root pointers and loads, shader memory, wave decoding and shader-cache checks. All GPU runs required Vulkan, disabled the application shader cache and loaded Khronos validation. No VUID/SYNC errors or skips; the fault fixture intentionally reports its BDA fault. Full build and conventions pass. This does not qualify #7392's barrier extension or a title. AI-assisted testing and independent review. |
What
Stopping a faulting invocation can leave wave ballots unable to converge and stop its healthy partner lane. Eligible non-pixel programs with two lanes per invocation, ballots or shuffles now keep voting with the failed read's zero and mask that lane's later writes, atomics and exports. Existing spanned-wave, barrier and mesh exclusions remain.
Continuing exposed a guarded-SRT reporting regression: the second word of a null pointer replaced the first fault record. Adopt ThatsLeo's first-fault latch from #7922, adapted so masked lanes count as continuing. The existing guarded-pointer runtime oracle stays unchanged, and its host checks require compare-from-zero latches without unconditional overwrites. No spanned-wave policy from #7922 is included.
Masking adds work even without faults. Astro Bot's reported startup delay remains unexplained; no performance claim. The barrier-safe extension and subgroup-specific skip remain separate in #7392 and #6751.
Tested
Based on
maine6f5730. Windows 11, MinGW-w64 GCC 15.2.0: Release, -DBUILD_TESTING=ON, SPIR-V Tools ON. Before repair, main passesagc_driver_guarded_null_pointer; the merged PR fails 3/3 at address4 instead of0, PC0x24. One diagnostic run confirms the overwrite.Repaired head
771c8bdae4: 13/13 focused processes pass: host shader-memory; NVIDIA guarded-pointer x3, faulting-wave, GPU-buffer-format, BDA-device and span-reads; the same five GPU tests once on Windows lavapipe. All GPU runs require Vulkan, disable application shader caching and load validation 1.4.363; zero skips, timeouts or validation diagnostics. Lavapipe subgroup8 does not establish paired wave64-on32 behavior. Test registration, conventions and source review pass.Earlier baseline
e2df06e661plus the faulting-wave fixture fails on main because partner lane37 stays untouched. Not tested: current-head Linux, AMD/Intel hardware, console behavior or titles.On the merge with main
98346b748c, the wave invocation case initially failed on the hardware-spanning wave path. The follow-up keeps those lanes active but masks writes after a fault, and guards byte loads when the lane continues. Six selected fault, guarded-load, wave-decode and cache tests passed twice on Windows with an NVIDIA RTX 3080 Laptop GPU and Vulkan validation enabled. The updated emitter, BDA read and shader-cache sources also passed Linux C++ syntax and layout-guard compilation. No Linux GPU run was performed.Checklist
main; the first-fault latch from perf(shader): span single-wave wave64 compute by default after a uniformity check #7922 is adopted with attributionNotImplemented_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)