Repository navigation
Conversation
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
The implementation and test report are thorough. The ten new lowering cases exercise the barrier placement and refusal boundaries, the GPU test checks output behavior, and the report includes negative controls plus a same-machine validation-layer run. I did not find a substantiated implementation defect in the reviewed diff.
Before merge, please refresh this branch from current main and rerun the focused tessellation set on the refreshed base and change. The reported runs use main 4777cac, while this PR is currently 54 commits behind the upstream tip. CONTRIBUTING.md requires shader recompiler changes to run 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, and compare the logs for new validation errors. The described runs satisfy this for their tested revision; current main still needs coverage.
The overlap bot identifies two still-open PRs that overlap this change: #6612 in tests/execution/Tessellation.cpp, and #8884 in tests/TessellationLowering.cpp / tests/execution/Tessellation.cpp. The body explains how the #8884 interaction can be integrated but the checklist says Depends on: none. Please coordinate the merge order with the affected authors and record the agreed dependency/order in the checklist, as the overlap rule requires.
Local checks: python tools/check_conventions.py --base upstream/main --head upstream/pr/9743 passed and git diff --check upstream/main...upstream/pr/9743 passed. I could not independently run the C++ or Vulkan tests: this checkout has no configured build and only about 649 MB free. Hosted product build jobs are skipped; hosted conventions, eligibility, intake, labels, and report checks pass.
|
Linux run, since the body says "Not run on Linux": llvmpipe (LLVM 23.1.1, 256 bits) via lavapipe, Mesa 26.2.4, Head. The three tessellation tests ( Negative control. AI-assisted: yes |
|
@Dextroid17 thank you for the Linux run, and for merging with current I corrected one number in the body. 0x1280659800 has 40 read-back loads after the barrier, not 48: I had counted that module by hand, and recounting it with the script I used for the others gives 40. The nine GT7 hull programs that this run met next are in #9806. They fail on a separate limit, an address that masks the control point out of |
MarcZX10
left a comment
There was a problem hiding this comment.
The existing evidence is strong: the PR reports paired Windows RTX 4090 and Linux lavapipe controls, repeated focused tests, spirv-val, and GT7 progress. The exact head 9c033796cb6938a453161b89d1e91f445f71c89e is based on 4777caca34, 75 commits behind current main 11f976b27c16ed91633f00039dba5dfd97dd7cc1; merge-tree/diff-check are clean. The PR body describes interaction with open #8884 and #6612, but the checklist still says Depends on: none, and I found no author-to-author order confirmation in this thread. Please coordinate the interaction with those authors and record the resulting order/dependency in the checklist, then rebase and rerun the focused tessellation tests plus required paired validation-layer comparison on current main and the final head. I did not independently run GPU tests locally.
…l-point-outputs # Conflicts: # core/libs/prx/libSceAgcDriver/tests/TessellationLowering.cpp # core/libs/prx/libSceAgcDriver/tests/execution/Tessellation.cpp
…ess in a hull program
|
@MarcZX10 thank you for the review. Both items are addressed, and the body is updated.
AI-assisted: yes (Claude Code) |
jotitadev
left a comment
There was a problem hiding this comment.
Apple M2 Max data point for head 7caf5085b.
Setup: Apple M2 Max, macOS 27.2, x86-64 under Rosetta 2, Release. Not main (it doesn't link on macOS): #8658 @ 91adbf7 (which includes #8884) + this PR's diff. Two drivers: MoltenVK 1.4.2 from the Vulkan SDK 1.4.363.0, and MoltenVK main with KhronosGroup/SPIRV-Cross#2704.
main() of agc_driver_tessellation stops at the first failing case, and on Apple the existing merged hull program case already fails before this PR's case runs. To see every case, I ran a local copy of tests/execution/Tessellation.cpp with each Require*/ExpectRefusal call wrapped in a try/catch.
Control (this PR's two test files only, without the lowering changes):
agc_tessellation_loweringfails; the new cases are refused withunsupported tessellation ring access at pc 0xc0.- With both drivers,
RequireBarrierBeforeRingReadBackand the newmerged hull program reading control point 2's level back from the off-chip ringcase both stop onunsupported tessellation ring access at pc 0x114, as in your control.
This PR, both drivers:
RequireBarrierBeforeRingReadBackpasses.- The new draw case fails:
merged hull program reading control point 2's level back from the off-chip ring, unculled: the level-1 patch was not drawn as one flat triangle, centre (16, 24, 40, 255). - That's the same failure as the existing
merged hull programandmerged hull program storing its local part's registerscases, which fail the same way on this base without this PR, with #8884 included. So on Apple the new draw case can't confirm the read-back, but the failure isn't caused by this PR. - Full
^agcsuite (-j1 --timeout 30, flip/display/lds_atomics64excluded): the same failing tests, with each driver, as an earlier base run without this PR (#8658 @ c5fa396, also with #8884).
GPUs tested: Apple M2 Max (MoltenVK 1.4.2; MoltenVK main + SPIRV-Cross#2704)
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
@MarcZX10 this branch now merges AI-assisted: yes (Claude Code) |
What
A hull program that reads control-point outputs of its own patch back from the off-chip ring threw
tessellation lowering: unsupported tessellation ring access. Only domain programs could read that ring.gl_outread. A read at a fixed offset reads control pointoffset / stride, and a read at the program's own control point's offset reads its own record. The stride comes from the program's ring writes, as before.s_barrierat pc 0x28, before their ring stores, and read the stores back after ans_waitcnt. A Vulkan tessellation control shader that reads another invocation's output needs anOpControlBarrierbetween the write and the read. In that execution model the barrier also makes the Output writes before it visible to the patch's other invocations (SPIR-V, OpControlBarrier). The modules use the GLSL450 memory model, where this implicit synchronization applies.s_barrieralready becomes in a hull program (OpControlBarrierWorkgroup, Invocation, None).v_mbcntcompared against the hull thread count in s3, thens_cbranch_execz.the hull shader reads another control point's off-chip output ...).mainsince, added an immediate-address path toTessellationPointerthat indexed every control-point output access by the reader's own control point. A hull read of another control point's output at an immediate address now takes that path too: it reads control pointaddress / stride, clamped to the last output control point, as the evaluated path does.gl_outafter the inserted barrier. It also says a field that no control point writes reads an undefined value instead of the ring's earlier contents.Why: Gran Turismo 7 (PPSA01317) stops on this refusal while loading the World Map, past the main-pass depth refusal (#9721):
shader 0x1280659800 stage 2: ShaderRecompiler::Recompile: tessellation lowering: unsupported tessellation ring access at pc 0x28c.(cp << 6) + (patch << 8)(4 output control points, 64-byte stride).v_cmpx_gt_u32 1, v20ands_cbranch_execzkeep control point 0, which loads fields of control points 0, 1 and 2 at fixed offsets 0x00, 0x40, 0x80 and 0x90 from the same ring.Tested
Refreshed onto
main0db47b2 (head 5da72b8; no conflicts), RTX 4090 (NVIDIA 616.92, Windows 11): the 52 ctest entries of the focused tessellation set below pass 52/52, each test run three times, and 52/52 under the validation layer (VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1,ANYPS5_REQUIRE_VULKAN=1,ctest -V), with noVUID-line; the layer reports nothing on this head.main0db47b2 passes the union of the refreshed sets, 115/115, under the same layer settings with noVUID-line.Gran Turismo 7, on a local stack of
mainwith the open PRs it needs before this stop (fix(agc): write a replace blend into a packed color target without blending #7243, feat(agc): render layered draws into a depth view of several slices #7256, fix(shader): answer image_get_resinfo on any T# dimension #7550, fix(agc): raise no end-of-pipe event from flips and carry the release's interrupt context #7978, feat(agc): merge partial writes into 4_4_4_4 UNORM color targets #8564, feat(agc): blend into 11_11_10 float color targets in the pixel shader #9020, feat(agc): draw alpha-tested packed-target merges that write depth as a depth pass and an EQUAL merge pass #9335, fix(shader): link table callees that together exceed s_call_b64 range #9068, feat(agc): draw EQAA color targets at their coverage sample count #9700, fix(agc): accept DB_EQAA INTERPOLATE_COMP_Z for a one-sample depth target without MSAA_ENABLE #9721) and this commit:Both hull programs, replayed with that stack's
agc_shader_replayfrom the requests the driver dumped when it refused them (APS5_DUMP_SHADERS):--preparenow succeeds for both.spirv-val --target-env vulkan1.2.OpControlBarriercomes after every control-point output store and before every read-back load: 0x1280659800 has 16 stores, the barrier, then 40 loads; 0x1280dcce00 has 47 stores, the barrier, then 56 loads.GT7 run with scripted pad presses, with its diagnostic switches that skip refused compute dispatches:
guest texture sampled with unnormalized coordinates is not a single-layer 1D or 2D view without a minimum LOD clamp, ... (base level 0, levels 10, layers 1, view type 1).the tessellation ring or LDS address is not affine in the control point and relative patch. Their control-point stores address(v1 & 0x1f00) + (patch << 10), which fix(shader): evaluate a control point masked out of the packed hull register #9806 handles. The run stopped before it used them.Windows 11 (build 26200), NVIDIA GeForce RTX 4090 (driver 616.92), MinGW GCC Release build with
BUILD_TESTING=ON, all on one machine.New tests:
agc_tessellation_loweringhas twelve new IR cases.!(LaneId < 4), does not keep the barrier from the writes. One that some control points take,!(LaneId < 3), is refused. Both cases number their block metadata apart from the IR's block ids, as translated programs do.agc_driver_tessellation(GPU) adds a merged hull program like the existing one, except that control point 0 reads control point 2's level for its tessellation factors from the off-chip ring instead of LDS, behind GT7's twos_cbranch_execz.Runs:
agc_tessellation_lowering_tests,agc_tessellation_factor_ring_tests,agc_driver_tessellation_tests,agc_driver_graphics_tests,agc_vector_window_tests,agc_vertex_input_validation_tests,agc_driver_bda_device_testsandagc_shader_memory_tests.main7e69be6): 52/52 passed, each test run three times (ctest --repeat until-fail:3).SpirvModuleEmitter.cppfrom before 7caf508.agc_tessellation_loweringfails on the two new cases: the read is indexed by the reader's own control point, which the test program never declares (SPIR-V module emission failed: stage input InvocationId was not declared).VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation(Vulkan SDK 1.4.363.0),ANYPS5_NO_SHADER_CACHE=1andANYPS5_REQUIRE_VULKAN=1withctest -V, onmain83462cc (7e69be6 plus eight commits totools/only) (52/52) and on this PR (52/52). Neither log haswas not found but was requested by env var VK_INSTANCE_LAYERS, and neither has aVUID-line.mainmerge (9c03379 onmain4777cac): 52/52 three times, and 52/52 under the layer with noVUID-line.maincontrol there:main4777cac with only this PR's two test files. Both tests fail. The ten new lowering cases are refused withunsupported tessellation ring access, and the GPU test stops on the new hull program with the same refusal at pc 0x114, its first ring load.main: the same results, including the control's stop at pc 0x114.tools/check_conventions.py --base origin/mainreports no findings.Checklist
main; no other open PR implements the same functions. fix(shader): accept a local tessellation index that nothing observes #6657 (a local tessellation index that nothing observes) changes another part ofTessellationLowering.cpp. feat(agc): draw tessellation followed by an NGG geometry stage #6612 (tessellation followed by an NGG geometry stage) changes the domain program's lowering; with this PR,TessellationLowering.cppandSpirvModuleEmitter.cppmerge cleanly, and both add GPU cases totests/execution/Tessellation.cpp, which conflicts. Neither needs the other; whichever lands second keeps both sets of cases (comment). fix(shader): index tessellation attributes at a constant address with constants #8884 is merged, and its immediate-address path is handled here.NotImplemented_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)Hull::mergeand cases at the end oftests/TessellationLowering.cpp, and fix(shader): evaluate a control point masked out of the packed hull register #9806 mergesmainonce this lands and keeps both. With feat(agc): draw tessellation followed by an NGG geometry stage #6612, whichever lands second keeps both sets of GPU cases. I've asked @milescoviello on feat(agc): draw tessellation followed by an NGG geometry stage #6612 to confirm this order.