Repository navigation
fix(agc): keep guest buffer offsets in shader data when stencil replay adds a push constant - #9427
TheSovietWaffle wants to merge 1 commit into
Conversation
…y adds a push constant Stencil replay appends a four-byte reference to a pixel shader's push constants. When the shader's binding layout keeps its shader data in a buffer (it does not fit the push range, or the shader uses runtime images), the reference is the only push constant. ShaderResources took any non-empty push constants to hold the guest buffer offsets, read them from the push constants and refused the draw. It now counts only the shader data part of the push constants, which ends at the stencil reference dword when the shader replays its stencil export. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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.
|
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
Confirmed red→green on Linux on two vendors (the PR's runs are both on Windows: RTX 3080 and an RDNA 2 iGPU).
Tested
Ubuntu 26.04, GCC 15.2.0, Release, BUILD_TESTING=ON, ANYPS5_NO_SHADER_CACHE=1 and ANYPS5_REQUIRE_VULKAN=1, device selected with VK_ICD_FILENAMES, run as ./agc_driver_stencil_replay_tests, on main a6dcea927 (this PR applies cleanly on it; head 62a5b6cea).
agc_driver_stencil_replay |
NVIDIA RTX 3070 (595.99.02) | lavapipe (Mesa 26.0.3, LLVM 21.1.8) |
|---|---|---|
main a6dcea927 + the PR's test only (ShaderResources.cpp reverted) |
fail | fail |
| this PR | pass | pass |
With ShaderResources.cpp reverted and the PR's new case kept, the run exits non-zero on both devices with:
AGC graphics: guest buffer offset lies outside the shader's push constants
That is the Require this PR relaxes: with the stencil-replay path the fragment push range is shrunk to the reference dword, so a s_buffer_load_dword of the color through a buffer descriptor in user data leaves the shader data in a buffer past the push constants. With the full PR, the new case ("dynamic reference keeps guest buffer offsets in shader data outside the push constants") colors every pixel from the buffer and passes on both devices.
GPUs tested: NVIDIA GeForce RTX 3070 (595.99.02, Linux) and lavapipe (Mesa 26.0.3, LLVM 21.1.8), Ubuntu 26.04.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
This reproduces a real failure and adds a useful buffer-backed regression. There is, however, an earlier open PR (#8615) that implements the same fix in the same ShaderResources::buildPrepare branch: it also replaces !push.empty() with a check of the shader-data bytes before stencilReferenceDword, and routes the no-shader-data case through dataByte. This is the same behavior, not just an adjacent edit, so the checklist claim that no open PR implements it is currently inaccurate. Please coordinate with #8615's author and the maintainers so the fix has one authoritative PR; if your buffer-offset variant adds distinct coverage, agree on transferring/combining that test without duplicating the implementation.
The reported test runs cover the regression on Windows across an RTX 3080 and an AMD RDNA 2 iGPU, but the repository requires the paired AGC validation-layer run with VK_LAYER_KHRONOS_validation loaded and a VUID- comparison. The description says that layer was unavailable. Please provide that paired run on a host where the layer is installed, and rerun the focused tests after updating from current main (the comparison base is a6dcea927, 53 commits behind the current tip).
The hosted eligibility, conventions, intake, labels, and report checks pass; Linux/Windows product jobs are skipped. A separate reviewer reports Linux Vulkan runs on two vendors, but those are on the same older a6dcea927 baseline and do not replace the required validation-layer comparison. I did not run the tests locally because no matching checkout/build is available here.
|
I'm the author of #8615. Its Your case covers a route that #8615's doesn't. #8615's shader gets a shader data buffer only through its image, which needs registration modules (the pipeline library). With I ran your case on top of #8615's change:
To keep one implementation, either of these works for me:
Which would you prefer? cc @MarcZX10 AI-assisted: yes (Claude Code). |
|
@Lindamea1 thanks, option 1 works for me: please add the test case to #8615 as a separate commit with me as its author. I'll close this PR once it's there. One note for the description: the case fails in both AI-assisted: yes (Claude Code). |
|
@TheSovietWaffle Done: your test case is now the second commit of #8615 (15803c7), with you as its author and its lines unchanged. I rewrote the commit message to describe only the test, since the fix is #8615's first commit. The description credits you and the Onimusha find (PPSA27836, #5769), includes your note on why the case fails in both tests ( #8615 is rebased onto
It's in, so this one can be closed whenever you're ready. Thanks for the find and the test. cc @MarcZX10 AI-assisted: yes (Claude Code). |
|
Closing: the fix is #8615 and this test is now its second commit (15803c7). Thanks @Lindamea1. |
What
On devices without
VK_EXT_shader_stencil_export, stencil replay appends a four-byte dynamic reference to the pixel shader's push constants (RecompileresizespushConstants,BindingAllocatorputsstencilReferenceDwordright after the shader data, or at dword 0 when the layout keeps its shader data in a buffer).The second case was broken. When a pixel shader's shader data does not fit its push range (or it uses runtime images),
IrBindingLayout::UsesPushData()is false and the guest buffer offsets live in the shader-data buffer; the stencil reference is then the only push constant.ShaderResourcesdecided where the guest buffer offsets are with!push.empty(), so it looked for them in the four-byte reference and refused the draw withguest buffer offset lies outside the shader's push constants.It now counts only the shader-data part of the push constants: all of them without stencil replay, and the bytes before
stencilReferenceDwordwith it (the same ruleRecompileand the prepared-artifact check already use). A layout without push data then takes the shader-data buffer path, as it does without stencil replay.Found while running Onimusha: Way of the Sword (PPSA27836) on an RTX 3080 with a local stack of the open stencil replay changes: the first replayed draw failed this way.
Tested
Windows 10 Pro 22H2 (19045), WinLibs GCC 15.2.0 r7, Release, on main
a6dcea927.agc_driver_stencil_replay, new case: the finite-export shader with its color read bys_buffer_load_dwordthrough a buffer descriptor in user data, with the fragment push range shrunk to the reference dword so the shader data stays in a buffer; the replayed draw must color every pixel from the buffer. On main it fails on both GPUs withAGC graphics: guest buffer offset lies outside the shader's push constants; with this change it passes.ANYPS5_REQUIRE_VULKAN=1andANYPS5_NO_SHADER_CACHE=1, on main and on this change:agc_driver_stencil_replay,agc_driver_stencil_replay_no_gpl,agc_driver_stencil_ref_export,agc_driver_stencil_ref_export_unused,agc_driver_stencil_known_replace,agc_driver_stencil_null_feedback,agc_driver_sample_mask_depth_stencil,agc_masked_select,agc_driver_graphics,agc_driver_draw_cache_relocation, the fiveagc_driver_draw_cache_reuse*variants,agc_driver_prepared_graphicswith its threeprebuildvariants,agc_driver_prepared_unregistered_graphicsandguest_buffer_reuse(20 tests) pass on both GPUs below, exceptagc_driver_stencil_ref_exportskipped on the RTX 3080 (noVK_EXT_shader_stencil_export). Results are identical on main and on this change (main runs its ownStencilReplay.cpp, without the new case).VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationhas nothing to load; neither log contains aVUID-line).tools/check_conventions.py --base origin/mainandgit diff --checkpass.tools/pr_overlap.pywas not run (noghon this host).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)1002:164e), 32.0.21045.5002, Windows 10 22H2🤖 Generated with Claude Code