Repository navigation
fix(shader): route a loop's back edge through an empty continue block when its latch holds instructions - #9746
Conversation
|
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.
|
|
Linux data point for Head. Negative control.
AI-assisted: yes |
MarcZX10
left a comment
There was a problem hiding this comment.
The reported regression test is well targeted: it includes the tested-header/latch-memory-access shape, checks that the structurizer produces an empty continue block, and the PR reports a negative control against the prior Structurizer. The reported validation-layer suite and title reproduction are useful evidence. I did not find a defect in the one-line structurizer change on this review.
Please refresh this branch from current main before merge and rerun the focused control-flow tests on that base and the refreshed change. The PR head is currently 50 commits behind main (base/head ancestry checked against current upstream/main). CONTRIBUTING.md requires keeping branches current and, for shader recompiler changes, running the same focused tests on main and the change with VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, ANYPS5_NO_SHADER_CACHE=1, and ANYPS5_REQUIRE_VULKAN=1 via ctest -V, comparing validation output and naming the GPU. The PR records strong runs on its earlier stack and a Linux main + this commit run, but does not establish that the focused paired comparison includes today’s main.
Local checks: python tools/check_conventions.py --base upstream/main --head upstream/pr/9746 passed; git diff --check upstream/main...upstream/pr/9746 passed. The local checkout has no configured build and only about 580 MB free, so I could not independently run the C++/Vulkan tests here. Hosted product build jobs are skipped; conventions, eligibility, intake, labels, and report jobs pass.
b425f0f to
8a1ae6b
Compare
|
Rebased onto Paired run on that base, same container and device for both sides: Ubuntu 24.04 x86-64, GCC 14.2, Release, lavapipe (Mesa 25.2.8),
@Dextroid17 thank you for the second toolchain and the six negative-control runs. You are right about the validation part: these four tests load no Vulkan loader, so "no validation messages" says nothing for them, and I have removed that claim from the description. The GPU-side evidence for this change is the Stray run on MoltenVK, where the shader that hung now runs. A paired AI-assisted: yes |
MarcZX10
left a comment
There was a problem hiding this comment.
The author’s follow-up provides the paired control-flow runs on main b7d1d47a5f and the rebased change, with the same focused tests and environment; it also explicitly notes those tests do not load Vulkan. The broader macOS validation-layer run and 200-second Stray reproduction provide useful additional evidence, with the same 22 known failures on both sides and no new layer messages. The exact head 8a1ae6b26806acab44cdff4db36b14ffac7ed71c has a clean merge tree and diff-check against current main 11f976b27c16ed91633f00039dba5dfd97dd7cc1, but is still 21 commits behind. Please refresh the branch and rerun the paired focused control-flow tests on current main and the final head. I did not independently rerun the GPU tests locally.
8a1ae6b to
3f8dc18
Compare
|
Rebased onto Paired run on that base, same container and device for both sides (Ubuntu 24.04 x86-64, GCC 14.2, Release, lavapipe from Mesa 25.2.8, validation layer 1.3.275,
AI-assisted: yes |
|
NVIDIA run on Linux, with the validation layer over the GPU tests: head
AI-assisted: yes (Claude Code). The runs are from this machine. |
|
@Lindamea1 thank you for the full GPU sweep: 554 tests recompiling their shaders on the RTX 5090 with the change, with the same results and the same 25 validation errors as AI-assisted: yes |
|
Thanks for refreshing the paired control-flow runs onto |
… when its latch holds instructions
3f8dc18 to
75fdf64
Compare
|
Rebased onto Paired run on that base, same container and device for both sides (Ubuntu 24.04 x86-64, GCC 14.2, Release, lavapipe from Mesa 25.2.8, validation layer 1.3.275,
The one validation message is the same on both sides: Ubuntu's 1.3.275 layer prints it at device creation because it does not know one structure in the device's AI-assisted: yes |
|
Tested on an Apple M2 Pro, macOS, x86_64 Release under Rosetta 2, MoltenVK 1.4.2, applied on top of #8658's
AI-assisted: yes (Cana) |
Dismissing a stale review: the requested changes were addressed after it (new head and/or updated description with the asked-for runs) and the merge queue blocks on it. Please request changes again if anything is still open.
What
On macOS, Stray (PPSA02100) loses the device about 30 s in: Metal reports
kIOGPUCommandBufferCallbackErrorHangfor the command buffer holding one draw, whose pixel shader (0x8011160000) is a uniform blur loop. Its guest code is simple: twos_buffer_load_dwords read a count (1020) and a step, and the loop runscount >> 1= 510 times, sampling one texel and adding it per iteration. The test is at the top of the loop and the work, withs_add_u32 s21, s21, 1, is in the latch.The SPIR-V we emit for it is valid (
spirv-valpasses) and correct: the header'sOpPhis take the incremented counter from the back edge. But the latch is the loop's continue target, and the image sample in it makes the emitter split it into six blocks, so the continue construct is%101 → %485 → %508 → %509 → %510 → %483and the back edge leaves from%483, not from the continue target. SPIRV-Cross turns that construct into afor (...; ...; increment)expression and drops the header'sOpPhiupdates from%483:_453never changes, so the loop never ends on Metal and the GPU watchdog stops it. The SPIRV-Cross in Vulkan SDK 1.4.363.0 and current SPIRV-Crossmain(300681b) both do this; Linux and Windows have no SPIRV-Cross in the path.Structurizer::canonicalizeNaturalLoopsalready routes the back edge through a dedicated empty continue block for loops with several latches, a latch in an inner loop, and single-block loops that hold instructions (9d8ee3d). This adds one more case: a single latch that holds instructions and has exactly one predecessor. Its instructions then stay in the loop body, and the continue construct is the empty block alone, which branches to the header. A latch that severalcontinuepaths share is left as it is: isolating it would clone its instructions into each path (the four "shared continue" programs inagc_shader_control_flowwould each gain 2 to 4 cloned instructions). That case is a new entry indocs/dev/debt/shader.md.Tested
macOS (Apple M4 Pro, macOS 27.0.1, MoltenVK 1.4.2, Vulkan SDK 1.4.363.0, x86-64 Release under Rosetta 2).
maindoes not build on macOS, so this ran onmain+ #8658 + #9193 + #9263 + #8612 + #8477 + #9356 + #9387 (with the three fixes from its review) + #9737, with this commit.agc_shader_control_flow,agc_shader_control_flow_iteration_selection_offandagc_shader_control_flow_forced_fallbackpass. New program "memory access in a loop latch behind a tested header" (the shape of Stray's shader) runs through the existing structural, execution-equivalence and recompile checks, and the newverifyEmptyLoopContinuesrequires its continue block, and the existing single-block loop's, to hold no instructions.Structurizer.cppfrommain,agc_shader_control_flowfails witha loop whose latch stores behind a tested header: the continue block 3 of the loop at block 1 holds instructions.ctest -R '^(agc|shader|request_serializer)'withANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation: 836 of 858 pass. The same 22 fail on the same stack without this commit, each with the same message, and the validation layer prints the same messages in both runs. They are MoltenVK limits (clip and cull distances, provoking vertex last, sampler reduction, multisample storage, tessellation, interlock) and two serialization tests that fail only because this local stack renumbers the request serialization versions of feat(recompiler): read I/J from barycentric built-ins on MoltenVK #9193, perf(shader): emit exact f32 add, subtract and multiply as fma on MoltenVK #8612 and feat(agc): run mesh-path draws through a compute pass on devices without mesh shaders #9356.APS5_SYNC_DRAWS=1 APS5_SYNC_DISPATCH=1, where the command buffer that "Caused GPU Hang Error" holds vertex 0x80122b0000 and pixel 0x8011160000); skipping only that pixel shader (APS5_SKIP_SHADER=8011160000) avoids it. With this change and nothing skipped, it presents frames for the whole 200 s run and draws the first-boot brightness screen correctly.Linux (Ubuntu 24.04 x86-64 container, GCC 14.2, Release, lavapipe from Mesa 25.2.8), on
main+ this commit, same environment variables:agc_shader_control_flow,agc_shader_control_flow_iteration_selection_off,agc_shader_control_flow_forced_fallbackandagc_shader_memorypass, onmain6a6a35fdf5and on this PR rebased onto it. These four tests load no Vulkan loader, so the validation layer does not run for them.NVIDIA (run by @Lindamea1): RTX 5090, driver 595.99.02, Gentoo, GCC 16.2.1, Release, layer 1.4.350, head
8a1ae6b268merged ontomain8499ea52e, same environment variables: the threeagc_shader_control_flowtests pass on both;^(agc|shader|request_serializer)(874 tests, 554 of which open the GPU and recompile their shaders with no cache) gives the same passes, failures and skip onmainand with the change, with the same 25 validation errors in the same tests; the fullctestwithout the layer (1834 tests) matchesmain.tools/check_conventions.py --base upstream/mainpasses.tools/pr_overlap.pyfinds no open pull request that changesStructurizer.cpportests/ControlFlow.cpp.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)