Skip to content

fix(shader): accept a local tessellation index that nothing observes - #6657

Open
xQuakes wants to merge 1 commit into
boykopovar:mainfrom
xQuakes:fix/shader-local-relative-index
Open

xQuakes wants to merge 1 commit into
boykopovar:mainfrom
xQuakes:fix/shader-local-relative-index

Conversation

@xQuakes

@xQuakes xQuakes commented Oct 16, 2026

Copy link
Copy Markdown

What

A local tessellation program that uses the relative patch index only to address local data share, and otherwise keeps it on inactive lanes or in a vector-window slot nothing reloads, was refused:

tessellation lowering: the relative patch or vertex index is used outside a tessellation ring or LDS address at pc 0x0

usedAsData treated every value that was not itself an address as a data use. The index is now followed through the arithmetic, selects, bitcasts and other pure operations that carry its bits. A side effect counts only when the value it observes still depends on the index. An exec-masked select counts only the arm that exec takes. A vector-window store counts only when a later load can see it. Storing the index into local data share, or reloading it from the window, is still refused.

PPSA12361 (Transport Fever 2: Console Edition) stops at startup on this refusal for shader 0x287f20000, stage 6.

#6551 also edits TessellationLowering.cpp, for a hull read at a control point chosen by a select or a phi. This change is the data-use check for a local program.

Tested

Windows 11, MinGW-w64 GCC 16.1.0, NVIDIA GeForce RTX 5090, driver 610.62.

  • agc_driver_tessellation_tests passes. A local program whose relative index survives only on inactive lanes compiles. Restoring exec before storing the index, and a window load after the address is stored, are still refused, as is the existing LocalIndexAsData case. The previous hull and domain cases pass.
  • The captured PPSA12361 request for shader 0x287f20000 compiles. The title gets past that shader and later stops on the multisampled storage-image refusal, which is feat(agc): write a multisampled color target through a storage image #5881.

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 under Silent stubs in their library's technical debt file
  • 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
  • Unblocks: PPSA12361 (no Game bring-up discussion yet)
  • GPUs tested: NVIDIA GeForce RTX 5090, driver 610.62, Windows 11
  • AI-assisted: yes

A local program that keeps the relative patch index only on inactive
lanes, or in a vector-window slot nothing reloads, was refused as a
data use. Follow the index through the values that carry it, and
refuse only when a side effect still observes it.
@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 6612 core/libs/prx/libSceAgcDriver/tests/execution/Tessellation.cpp
core/shader/recompiler/Optimization/src/TessellationLowering.cpp
PR 7642 core/libs/prx/libSceAgcDriver/tests/execution/Tessellation.cpp
PR 9508 core/libs/prx/libSceAgcDriver/tests/execution/Tessellation.cpp
PR 9743 core/libs/prx/libSceAgcDriver/tests/execution/Tessellation.cpp
core/shader/recompiler/Optimization/src/TessellationLowering.cpp
PR 9806 core/libs/prx/libSceAgcDriver/tests/execution/Tessellation.cpp
core/shader/recompiler/Optimization/src/TessellationLowering.cpp
PR 10193 core/libs/prx/libSceAgcDriver/tests/execution/Tessellation.cpp
PR 10249 core/shader/recompiler/Optimization/src/TessellationLowering.cpp

@jotitadev jotitadev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linux run on NVIDIA and lavapipe, which the Linux CI job uses.

Machine: NVIDIA GeForce RTX 3070, driver 595.99.02, Vulkan 1.4.329, plus lavapipe (Mesa 26.0.3, LLVM 21.1.8) on the same machine. Ubuntu 26.04, i7-8700, GCC 15.2, Release, BUILD_TESTING=ON. Head 41972eeb9 merged into main 5d9dcc42d (clean merge), -j1 --timeout 30.

  • Tests only (TessellationLowering.cpp restored from main): agc_driver_tessellation fails on both devices with tessellation lowering: the relative patch or vertex index is used outside a tessellation ring or LDS address at pc 0x0.
  • With the fix: agc_driver_tessellation and agc_tessellation_factor_ring pass 3/3 on the RTX 3070 and 3/3 on lavapipe.

GPUs tested: NVIDIA GeForce RTX 3070 (595.99.02), 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.

@github-actions github-actions Bot added shader core/shader agc libSceAgc* labels Oct 16, 2026
@savvaniss

Copy link
Copy Markdown
Contributor

Intel and AMD Windows data: the new cases pass on both GPUs with this change and fail on both without it.

Windows 11 Pro 26300, WinLibs GCC 15.2.0 r7, Release, every execution test built, ANYPS5_NO_SHADER_CACHE=1, serial CTest. Intel UHD Graphics 770 (8086:a780), driver 32.0.101.7088. AMD Radeon RX 9070 XT (1002:7550), driver 32.0.31041.1004, with VkCache emptied first. "Without" means this PR's test files on main c9e6c45de, with none of its other changes.

UHD 770 RX 9070 XT
This head 41972eeb9 merged into main ca82d7d88: agc_driver_tessellation, ×3 pass pass
Without, ×3 fails: tessellation lowering: the relative patch or vertex index is used outside a tessellation ring or LDS address at pc 0x0 fails, same

Full CTest (--parallel 4) on main c9e6c45de with seven open GPU PRs merged (#6258, #6657, #6324, #6246, #6174, #6360, #6598): UHD 770 1381 of 1389, RX 9070 XT 1383 of 1389. main alone passes 1378 of 1385 and 1381 of 1385. The merge adds only two failures: agc_buffer_pool (#6174, both GPUs) and agc_driver_indirect_dispatch_limit (#6360, RX 9070 XT).

AI-assisted: yes (Claude Code)

@klosixgaming-arch

Copy link
Copy Markdown
Contributor

Tested on Windows 11, MinGW-w64 GCC 15.2.0, NVIDIA GeForce RTX 5090 (driver 617.42), Ryzen 9 9950X3D. Head 41972eeb9 merged onto current main (a373e85); the merge is clean.

  • agc_driver_tessellation passes with the change.
  • On main with only this PR's test file (tests/execution/Tessellation.cpp) the same test fails with tessellation lowering: the relative patch or vertex index is used outside a tessellation ring or LDS address at pc 0x0, the refusal quoted in the description. So the new cases do exercise the fix.

Not tested: PPSA12361 (no dump here). I skimmed dependsOnRelative and windowLoadObserves but did not check their soundness line by line. Since this relaxes a refusal, a mistake would silently accept a real data use of the index; the negative cases in the test (restored exec before the store, window load after the address store, LocalIndexAsData) still pass, but they only cover the shapes the author thought of. I am leaving this as a test report rather than an approval.

AI-assisted: yes. Written with AI assistance and reviewed by me.

@adriadam10 adriadam10 self-assigned this Oct 19, 2026

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change and the thorough third-party runs. One correctness problem blocks it:

  • core/shader/recompiler/Optimization/src/TessellationLowering.cpp:256 (ImpliesNot): for predicate = !p it returns Implies(p, condition). p => condition does not give !p => !condition. Example: p = a && b, condition = a, predicate = !(a && b). With a = true, b = false the predicate holds and the select takes its first arm, but dependsOnRelative follows only the second arm and can accept an index that is observed. The sound cases are p == condition (then !p => !condition), or the other direction, condition => p. Please fix it (probably only the equality case, since Implies only looks at LogicalAnd on the left) and add a test whose masked-select predicate is a negated conjunction where the observed arm carries the index, which must stay refused.

The rest of the diff (the CarriesRelativeBits set, the window-load reachability walk, the existing refusals kept) reads fine.

@DavidAmsellem

Copy link
Copy Markdown
Contributor

First AMD-on-Linux (RADV) run of this PR. Merged onto main dbac9fa15 (clean merge), built in full and compared with the same main, on the same machine.

AMD Radeon RX 7900 XT (RADV NAVI31, gfx1100), Mesa 25.2.8, Linux Mint 22.3 (Ubuntu 24.04 base), kernel 7.0.0-38, GCC 13.3, Release.

Full suite (ANYPS5_REQUIRE_VULKAN=1 ctest -j8): 1638/1640 with this PR, against 1638/1640 on main. The failures are the same two on both, so this PR adds none: agc_driver_image_table_vouched_lookups (#7766, fixed by #7916) and agc_driver_guest_memory_lifetime (#7784).

agc_driver_tessellation, agc_tessellation_lowering and agc_tessellation_factor_ring run and pass. The PR adds no new ctest entry, and no test is skipped or removed compared with main.

GPUs tested: AMD Radeon RX 7900 XT (RADV NAVI31, gfx1100), Mesa 25.2.8, Linux Mint 22.3 / kernel 7.0.0-38

AI-assisted: yes

@DavidAmsellem

Copy link
Copy Markdown
Contributor

First AMD-on-Linux (RADV) run of this PR. Merged onto main dbac9fa15 (clean merge), built in full and compared with the same main, on the same machine.

AMD Radeon RX 7900 XT (RADV NAVI31, gfx1100), Mesa 25.2.8, Linux Mint 22.3 (Ubuntu 24.04 base), kernel 7.0.0-38, GCC 13.3, Release.

Full suite (ANYPS5_REQUIRE_VULKAN=1 ctest -j8) at 4779e727d: 1638/1640 with this PR, against 1638/1640 on main. The two failures are the same on both, so this PR adds none: agc_driver_image_table_vouched_lookups (#7766, fixed by #7916) and agc_driver_guest_memory_lifetime (#7784).

Comparing the JUnit output with main: this PR adds no ctest entry, and none is skipped or removed.

This checks correctness only; I did not measure performance.

GPUs tested: AMD Radeon RX 7900 XT (RADV NAVI31, gfx1100), Mesa 25.2.8, Linux Mint 22.3 / kernel 7.0.0-38

AI-assisted: yes

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 20, 2026
@MarcZX10

Copy link
Copy Markdown
Contributor

One checklist item remains incomplete independently of the code concern in the existing changes-requested review. CONTRIBUTING.md requires a title unblock to name both the title ID and its Game bring-up discussion. The current line says Unblocks: PPSA12361 (no Game bring-up discussion yet), so it does not meet that format. Please add the discussion link if this PR is intended to unblock the title, or use none if it is not.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* needs rebase Conflicts with main, rebase needed shader core/shader unblocks title Names a title ID on the Unblocks line

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants