Skip to content

fix(shader): isolate terminating paths from loop continuations - #2311

Closed
JulioCacko wants to merge 1 commit into
boykopovar:mainfrom
JulioCacko:contrib/spirv-loop-continuations
Closed

JulioCacko wants to merge 1 commit into
boykopovar:mainfrom
JulioCacko:contrib/spirv-loop-continuations

Conversation

@JulioCacko

Copy link
Copy Markdown
Contributor

What

Runtime image operations can emit terminating paths inside a guest loop's continuation. Record emitted OpLoopMerge offsets and move the SPIR-V continuation to a separate backedge block only when the emitted instruction range contains OpUnreachable, OpReturn, OpReturnValue, OpKill, or OpTerminateInvocation. Preserve the semantic guest CFG and patch phi predecessors to the actual block exit labels.

This is a SPIR-V structural correction, not a change to RDNA instruction semantics. The rule comes from the SPIR-V specification's structured-control-flow restrictions on continue constructs: https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#_structured_control_flow. Tests cover patch offsets across vector growth, invalid offsets/IDs, terminating opcodes, unchanged guest instruction ranges, and a runtime image loop's three stores plus untouched exit row in wave32 and wave64.

Tested

Windows 11 x64, MinGW GCC, RelWithDebInfo, SPIRV-Tools enabled, NVIDIA GeForce RTX 3070. Focused build passed; agc_spirv_module, agc_structurizer, and agc_driver_runtime_images passed 3/3 (0.15 s, 0.14 s, 0.80 s) with ANYPS5_REQUIRE_VULKAN=1, no skips. The branch/title/body conventions check passed. Built and tested with the integrated contribution set on upstream d70b8998; this branch contains only the loop correction. Synthetic regression results do not establish Demon's Souls presentation or gameplay.

Checklist

  • Based on current main; open-PR overlap checked before submission
  • One topic per PR
  • No new code comments or investigation files in the repository
  • Generic behavior; no silent stubs or new third-party code
  • Depends on: none
  • AI-assisted: yes; reviewed and submitted by the author

@Kikoarl

Kikoarl commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Tested on Linux (CachyOS x86_64, Intel RPL-S iGPU / Mesa 26.3.0-devel / Vulkan 1.4.363, GCC 16.2.1) at fc56e7b3:

  • Build: Built cleanly in RAM (/tmp/anyps5_build) with zero warnings/errors.
  • Conventions: python3 tools/check_conventions.py --base origin/main --head HEAD -> PASS (0 errors).
  • Verification: Verified SpirvModule.cpp, Structurizer.cpp, and RuntimeImages.cpp. Isolates terminating blocks from loop continuations, generating valid SPIR-V control flow graphs for Vulkan validation.

LGTM!

@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. I built it merged onto current main and ran the full libSceAgc/libSceAgcDriver ctest (293/293 green), but the new tests don't show the bug:

  • tests/execution/RuntimeImages.cpp:22 (RunLoop, wave32 and wave64): with the four SpirvBackend sources reverted to main, agc_driver_runtime_images still passes on RADV. The loop it builds never yields a terminating path inside a continuation on main, so neither the structural check (lines 53-62) nor the stored values (lines 67-74) exercise the change.
  • tests/Structurizer.cpp:81: the PR changes nothing in the structurizer, so this case passes on main as well and tests nothing of this PR.
  • Only tests/SpirvModule.cpp (the new helpers) depends on the change, and it pins the helper API, not the failing case.

Please add a case that fails on main: a guest loop whose shader (a runtime image op, as you describe) really emits OpUnreachable/OpKill/... in the continue block, and check it fails on main (e.g. spirv-val on the output, ANYPS5_ENABLE_SPIRV_TOOLS, or a before/after log of the validation error on your RTX 3070). Also state which driver rejects the main SPIR-V.

Smaller point: core/libs/prx/libSceAgcDriver/CMakeLists.txt:701 adds the include directory for the test outside the if(BUILD_TESTING) block and not next to the target definition; move it there.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 10, 2026
hypermoist added a commit to hypermoist/AnyPS5 that referenced this pull request Oct 10, 2026
@Mouhand-Kaddo

Copy link
Copy Markdown
Contributor

Tested head fc56e7b39159 against current main 2b831510be94 on both platforms, with the repository's pinned SPIRV-Tools enabled:

  • Windows 11 / RX 6900 XT / AMD 32.0.21045.5002 / MinGW GCC 15.2 / Release: agc_structurizer, agc_spirv_module, and agc_driver_runtime_images 3/3 passed, no skips (1.14 s).
  • Arch Linux in WSL2 / lavapipe / GCC 16.2.1 / Release: the same tests 3/3 passed, no skips (0.41 s). This Linux result covers software Vulkan, not a native hardware driver.

Both runs used the default pipeline configuration; APS5_NO_PIPELINE_LTO was unset.

The branch needs an update for current main. For this local validation I resolved the conflicts in SpirvModule.hpp by retaining main's helper-function API alongside the new loop-patching API, and in SpirvFlowEmitter.cpp by retaining main's continueTarget handling alongside the new instruction-range tracking. The new RunLoop fixture also needs the fifth gpu argument in GuestAllocations::Mutation().Add(...); I used true for its GPU-written texture allocation. These adaptations were needed before the test results above could be obtained.

hypermoist added a commit to hypermoist/AnyPS5 that referenced this pull request Oct 11, 2026
@adriadam10

Copy link
Copy Markdown
Collaborator

Thanks for the work. I rebased this onto current main and tried to get a case that fails without it:

  • tests/execution/RuntimeImages.cpp (RunLoop): with the four SpirvBackend sources reverted to main, the test still passes. I also moved the image op into the latch block of a do-while loop (the continue target itself); it still passes on main, structural check included.
  • The runtime image case is already handled on main by 33603bf ("keep runtime image mode switches inside continue targets"): EmitImage reads state.continueTarget and doesn't emit OpUnreachable there, so no terminating path ends up in a continue construct. This PR adds a second mechanism (separate back-edge block + OpLoopMerge patching) for the same case.
  • The remaining new tests (SpirvModule.cpp, Structurizer.cpp) only pin the new helpers, not a failing shader.

Closing as a duplicate of work already on main. If you have a shader that still produces a terminating op inside a continue construct on current main (a spirv-val error or a driver rejection, with the driver named), please reopen or open a new PR with that case as the test.

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

Labels

agc libSceAgc* build needs rebase Conflicts with main, rebase needed shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants