Repository navigation
feat(shader): link raw vertex prologs through user-data tails - #4821
Conversation
|
@TibixDev @eljackjoestar-cmd Coordinating the bot's overlap with #1313 and #4271, also inherited by my follow-up #4840. Pairwise
I propose landing #1313 and #4271 before #4821, then #4840; #4715 remains the separate title-bring-up prerequisite. I am listing the first two as proposed merge-order dependencies, not runtime requirements or an order already agreed with you. After they land, I will rebase both tail PRs and rerun the relevant shader/draw tests, including the vertex and fragment pixel regressions. These are merge simulations, not a combined build or runtime validation. Does that order work for you? AI-assisted: yes (Codex). |
|
Yes, that order works for #4271: #1313 and #4271 first, then #4821, then #4840. |
57c761a to
c3882cf
Compare
c3882cf to
05ba2e4
Compare
adriadam10
left a comment
There was a problem hiding this comment.
Thanks, the approach (recognise a terminal s_setpc_b64 through an unchanged user-data pair, link the continuation with a relative branch) looks right. One blocker:
core/libs/prx/libSceAgcDriver/tests/execution/VertexUserDataTail.cpp:118and:122: the SCC/EXEC cases require every pixel to stay untouched, i.e. a vertex shader that never reaches its position export must draw nothing. The position is undefined in that case and the spec doesn't guarantee that. On RADV (Radeon 660M, Mesa)agc_driver_vertex_user_data_tailfails withSCC was not preserved across the tail jump(the linked and monolithic shaders still match each other, so the draw itself is consistent); on CI's lavapipe 25.2.8 it passes. Please make these rows driver-independent: e.g. have the skipped path still export a valid position and compare the colour written by each branch (the taken path writes a different value than the skipped one), instead of asserting that nothing is drawn.
Also needs a rebase: docs/dev/debt/shader.md conflicts with main (keep both entries).
Checked: build and the PR's tests in libSceAgc/libSceAgcDriver ctest scope (all pass except the one above; agc_driver_tessellation segfaults on main too), the new test fails without the source change.
|
Rebase handoff — rebased onto current One finding — independent of the rebase: Verified on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ninja, Release,
To pick it up: |
05ba2e4 to
7092b01
Compare
|
@MichonGoddijn231849 coordinating the overlap between #494 and my #4821 / #4840. The reports for #1191, #1193, #1201 and #1202 inherit the same cache changes through your stack, so I suggest keeping the discussion here. I checked #4821 For this overlap, I propose #494 first, then #4821, then #4840, subject to the other prerequisites already listed on my PRs. Your follow-ups can keep their existing dependency chain; this does not make the entire performance stack a prerequisite for the tail fixes. Once we agree on the order, I will record the direct dependency in The integration needs more than keeping both sets of fields:
These are merge simulations and a source review; I have not built or run the combined stack. Does this order work for you? If you prefer the tail fixes first, we can agree on that and document who handles the integration. AI-assisted: yes (Codex). |
|
@blawesom @adriadam10 thanks for the RADV checks and the rebase handoff. Both branches are now rebased, and the SCC/EXEC test has been revised at #4821 Both SCC outcomes now export a valid position and a defined white/black vertex color. The EXEC cases save the active mask, perform the masked color write, then restore the mask before exporting color and position. The test compares the linked and monolithic images and checks the expected colors; it no longer expects a vertex without a position export to draw nothing. Windows NVIDIA: 18/18 focused tests pass for #4821 and 19/19 for #4840. Linux lavapipe: #4821 validation and #4840 validation pass, with the separate clip/cull-distance test skipped on lavapipe. At the #4821 head, deliberately corrupting only the linked SCC or EXEC fails the new pixel comparison on both platforms; restoring the source passes. I have not retested on AMD hardware. Could you rerun AI-assisted: yes (Codex). |
adriadam10
left a comment
There was a problem hiding this comment.
Checked the head 7092b01b0 merged onto current main:
agc_driver_vertex_user_data_tail(linked vs monolithic pixels, SCC/EXEC cases, changed and rewritten continuation, clip/cull from the continuation) passes on RADV (gfx1035) with Vulkan required;agc_shader_control_flowandagc_driver(the newtestVertexTailSnapshots) pass. Without the change the new tests don't build.- On hardware (gfx1035), a jump through
s_setpc_b64to an address built withs_getpc_b64keeps SCC, EXEC and VGPRs, which matches linking with a relative branch. - The earlier SCC/EXEC test concern is addressed by the revised test.
Merging once the full Build is green.
What
XashPS5 (PPSA19111, v1.3 / content 01.030.000, bring-up #4711) uses a separate RADV vertex prolog. After #4715 removes the point-size state stop, its prolog jumps to the main vertex shader with
s_setpc_b64 s[8:9]. The target pair comes from user data and is unchanged by the prolog. The current recompiler rejects that transition:Recognize terminal jumps through an unchanged user-data pair in a linear raw vertex prolog, read the continuation, and link it with a relative branch. Preserve the prolog's registers, EXEC and SCC. Include the continuation in draw-cache validation so rewriting its code cannot reuse a stale draw. The workgroup check also inspects the continuation; programs requiring the unsupported mesh-tail path still stop instead of silently taking the vertex path. Ordinary shaders cache the absence of a tail and keep their existing workgroup check. The rebase also retains main's position-vector cache from #423 and inspects the continuation when determining the clip/cull export layout, so distances exported only after the tail jump are preserved.
Computed targets, branching prologs, registered shader tails, fragment epilogs and continuations that read their relocated PC remain unsupported. No fallback, shader skip or title-specific address is introduced.
The instruction contract is from AMD RDNA2 ISA, sections 12.3 (
S_SETPC_B64) and 12.5 (S_BRANCH). The observed convention is in XashPS5's pinned Mesa vertex prolog and RADV prolog inputs. This has not been measured on console hardware; the limitation is recorded in shader debt.Tested
Windows: Windows 11 Pro 26100, WinLibs MinGW-w64 GCC 15.2.0 r7, Ninja, Release,
BUILD_TESTING=ON, GTX 1660 SUPER (driver 32.0.15.5585). Fulllibs,relinkerand focused targets build at7092b01b0daab6afc52ce67d79f5d0f2537b7279, based on maindacd78525e537a779682032c0ace7f7af120af05. LocalANYPS5_ENABLE_SPIRV_TOOLS=OFF. All 18 selected tests pass with Vulkan required: graphics, continuation clip/cull exports, layered rendering, shader memory/control flow, driver, scalar set-PC, compute user-data calls and prepared graphics.Portable vertex state regression, following review: both SCC outcomes export a valid position and a defined vertex color. The EXEC cases save the active mask, perform a masked color write across the tail boundary, and restore the mask before exporting color and position. Full-image comparisons check linked versus monolithic shaders, identical triangle coverage and white versus black pixels. No assertion relies on vertices that omit a position export. Separate fault-injection checks that corrupt only the linked SCC or EXEC each fail the pixel comparison; restoring the source passes. These checks pass on Windows NVIDIA and Linux lavapipe. AMD RADV hardware has not been retested here.
Other regressions: repeated draws, changed tail addresses, rewritten/restored code and continuation-only clip/cull distances remain covered. Driver/parser cases retain invalid targets, overwritten registers, relocated-PC reads, cache invalidation and workgroup detection. The pixel test fails against main at
unsupported dynamic s_setpc_b64on Windows and Linux. Linux also confirms that removing continuation-aware position-layout detection fails the clip/cull pixel comparison.Linux: exact-source validation passes on Ubuntu 24.04, GCC, Release, default
ANYPS5_ENABLE_SPIRV_TOOLS=OFFand lavapipe, with Vulkan required. Fulllibs/relinkerand focused targets build. Of 18 selected tests, 17 pass andagc_driver_vertex_clip_cull_distancesskips; zero failures. That test passes on the Windows GPU. The workflow verifies the negative controls, restores the exact source, rebuilds and retests. Conventions, test registration and patch whitespace pass. These are focused checks, not the full suite. The validation workflow is kept off the PR source branch.Title, Windows: on main
43e3a70f8, the same installed Half-Life 1valvedata stops at the original point-size check. With fix(agc): accept inactive vertex point size on non-point draws #4715 alone (local integrationce37e254c) it reaches the vertex tail error above. With this change plus fix(agc): accept inactive vertex point size on non-point draws #4715 (82b64a3da, clean local integration with the exact final shader changes), it passes that vertex transition and stops in the fragment shader:The run exits after 5.27 s, without a timeout or rendering bypass. The fragment shader overwrites the high half of its continuation address, so it intentionally does not match this rule. Menu, gameplay and FPS improvements are not claimed.
Conventions, test registration and patch whitespace checks pass. This is focused validation, not the full test suite.
Title reproduction uses the public v1.3 release,
relinker --windows --windows-diagnostics --registry input/eboot.elf run/app.exe, bundled guest libc, runtime libraries inrun/libs, release resources plus owned Half-Life 1 data inrun/app0/valve, thenrun/app.exefromrunwithout arguments. No game data is included in this PR.Related work and overlap coordination
Rebased onto main
dacd78525, including merged #3763 and #4271. Both shader-debt entries are retained. Position-export layout resolution retains the continuation together with routed layer bits. The tests above cover that integration.#4715 remains the XashPS5 bring-up prerequisite; its commits are not included. #4706 concerns compute
s_swappc_b64, a different operation.Previously coordinated overlaps are #1313 (
bff970af5) and #4943 (df467c2b1). Proposed order: those PRs, then #4821, then #4840; agreement is not claimed. PreservetailContinuation,vertexInputsandhiddenUserWords, plus continuation-aware position layouts and layer bits, when integrating them. Their implementations are not carried here; no combined runtime validation is claimed. The coordination thread tracks these overlaps. #4271 is now merged, so it is no longer an outstanding dependency.Additional overlap: #494 and its stacked follow-ups #1191, #1193, #1201 and #1202 share draw-cache changes. Coordination proposes #494, then #4821, then #4840 for this overlap, subject to the existing prerequisites. Agreement is pending; the direct
Depends onentry will be updated once the order is agreed. Pairwise merge simulations at #4945fe165d37and #1202fda7dbf04conflict inDrawCache.hppandDrawDecode.cpp. Integration must preserve continuation snapshots and their validation, refresh the continuation after data-only user-word reloads, and retain clip/cull layouts. Combined build/runtime validation remains outstanding; the standalone results above do not cover this stack.The revised SCC/EXEC test is also covered by the RADV retest request; AMD hardware validation is still pending.
The title evidence above is from the pinned earlier runs, not a fresh title run on this rebase. No menu, gameplay or FPS improvement is claimed.
Checklist
main; no other open PR found implementing this vertex-tail fix