Repository navigation
fix(shader): evaluate a control point masked out of the packed hull register - #9806
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
The masked-field lowering is narrowly constrained to a contiguous bit run, and the two refusal tests cover masks that drop a control-point bit or contain a gap. The report gives useful GT7 shader replays, output tests, negative controls, and exact-head conventions/diff checks passed locally here.
Two repository-readiness items remain:
- The PR itself identifies open #9743 as touching the same
TessellationLowering.cppandTessellationLowering.cpptest file, but saysDepends on: noneand does not record an agreed merge order. Please coordinate with #9743’s author and record which branch goes first (or confirm the explicit no-dependency resolution while preserving both changes), as the overlap rule requires. - For the shader recompiler change, CONTRIBUTING requires the same focused Vulkan tests on
mainand the change with the validation layer enabled, and a comparison showing no newVUID-errors. The description reports the validation-layer run on this PR and amainnegative-control run without saying that the main control used the layer or reporting its VUID comparison. Please add that baseline result, or explain if the control exits before Vulkan is initialized.
I did not run the C++/Vulkan suite locally; there is no configured build tree in this checkout. Hosted product tests are skipped. The remaining review requests are limited to overlap coordination and the required baseline validation evidence.
|
Small wording correction to my review: the overlapping files are |
|
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 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. Overlap: AI-assisted: yes |
|
@Dextroid17 thank you for the Linux run, and for resolving the @MarcZX10 both items:
AI-assisted: yes (Claude Code) |
jotitadev
left a comment
There was a problem hiding this comment.
First run on Apple silicon.
Setup: Apple M2 Max (32 GB), macOS 27.2, x86_64 build under Rosetta, Release, BUILD_TESTING=ON. Method: #8658 (feat/macos-port) @ dc0c8db500 plus this PR's diff (head c46b559897), not main, since main does not build on macOS yet. Two drivers: Vulkan SDK 1.4.363.0 MoltenVK 1.4.2, and MoltenVK main with SPIRV-Cross #2704. ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1, ctest -R tessellation -j1 --timeout 30 -V.
| SDK MoltenVK 1.4.2 | MoltenVK main + #2704 | |
|---|---|---|
| test files only (red) | agc_tessellation_lowering fails (the new masked case is refused as not affine); agc_driver_tessellation stops at the new draw with the tessellation ring or LDS address is not affine in the control point and relative patch at pc 0x60 |
same |
| this PR (green) | agc_tessellation_lowering and agc_tessellation_factor_ring pass; agc_driver_tessellation passes the new HullMaskedControlPoint draw |
same |
agc_driver_tessellation still fails on Apple with this PR, later in main(), at merged hull program, unculled: the level-1 patch was not drawn as one flat triangle, centre (16, 24, 40, 255). That case fails the same way on the #8658 base without this PR, so it is not from this change.
GPUs tested: Apple M2 Max (MoltenVK 1.4.2 from Vulkan SDK 1.4.363.0, and MoltenVK main + SPIRV-Cross #2704), macOS 27.2.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
MarcZX10
left a comment
There was a problem hiding this comment.
The latest description resolves the two issues in my prior review: it records that #9806 follows #9743 and reports the same 52-test validation-layer set on main and the change, with no missing-layer warning or VUID- lines; the Linux lavapipe comparison also reports 0 new VUIDs. The reported Windows and Linux GPU test coverage is substantial, and the local diff check, conventions, and test-registration checks pass. I could not independently run the Vulkan tests in this host environment.
One repository-readiness blocker remains: tools/check_main_sync.py --base upstream/main --head <head> reports this branch 2 commits ahead / 194 behind current upstream/main (merge base 7e69be679), though it merges cleanly. CONTRIBUTING requires branches to be based on and kept up to date with main. Please merge current main, preserve the declared ordering with #9743, then rerun the focused tessellation suite and validation-layer comparison on the combined head. This is a freshness request only, not an intake or queue-size objection.
Apply PR boykopovar#9806 at 2945108 and retain both its address-mask regressions and PR boykopovar#9743 control-output tests.
Apply PR boykopovar#9806 at 2945108 and retain both its address-mask regressions and PR boykopovar#9743 control-output tests.
Apply PR boykopovar#9806 at 2945108 and retain both its address-mask regressions and PR boykopovar#9743 control-output tests.
|
AMD run, on Windows next to NVIDIA, on current
AI-assisted: yes (Claude Code); results above were run on this machine. |
2945108 to
724cabc
Compare
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Rebased onto current |
Replaces #9804, which the open PR limit check closed after its merged-PR search returned 0 for one run (I have 2 PRs merged into
mainand 10 others open).What
The tessellation lowering evaluates every ring and LDS address as
vertex * control point + relative * relative patch + constant. Av_and_b32with a constant was accepted only when the mask kept the low bits, like0xffor0x1fff. A hull program'sv1packs the control point into bits 8-12, above the relative patch in bits 0-7. A mask like0x1f00, which keeps only the control point, was refused withthe tessellation ring or LDS address is not affine in the control point and relative patch.(x mod 2^hi) - (x mod 2^lo).split. It proves each one exact: the part below the boundary must stay inside the boundary for every control point and relative patch.v1 & 0x1f00iscontrol point * 256, as long as the relative patch stays below 256 and the control point below 32.0x1e00drops the control point's low bit, and0x1f0fhas a gap.Why: Gran Turismo 7 (PPSA01317) has nine hull programs that address their control-point records in the off-chip ring this way: 0x1280676200, 0x1280ada900, 0x1280c98400, 0x1280d71100, 0x1280ed4e00, 0x1280ed8d00, 0x1280f63d00, 0x12810bce00 and 0x12810d5900. Their control-point records are 256 bytes.
(v1 & 0x1f00) + relative patch * patch stride. The patch stride is<< 10(4 control points) in eight of them and* 0x300(3 control points) in 0x1280ed8d00.v_and_b32 v0, 0x1f00, v1, thenv_lshl_add_u32 v0, v46, 10, v0withv46 = v1 & 0xff, thenbuffer_store_dwordx4 ... offen offset:0x68at pc 0x2d0.Tested
Refreshed onto
main0db47b2 (head 2945108; 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 these hulls (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, fix(shader): read other control points' off-chip outputs in hull programs #9743) and this commit:The nine hull programs, replayed with that stack's
agc_shader_replayfrom the requests the driver dumped when it deferred them (APS5_DUMP_SHADERS).v1with0x1f00.--preparesucceeds for all nine, and the recompiled SPIR-V passesspirv-val --target-env vulkan1.2.GT7 runs 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 ...).... PS_ITER_SAMPLE, a sample index or coverage input, a sample-mask export ...), before GT7 creates any tessellation pipeline.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 three new IR cases.((packed & 0x1f00) >> 3) + relative patch * stride, wherepackediscontrol point << 8 | relative patchas inv1, reads the invocation's own input control point.packed & 0x1e00, which drops the control point's low bit, andpacked & 0x1f0f, which has a gap.agc_driver_tessellation(GPU) addsHullMaskedControlPoint. It isHullCodewith its off-chip record address16 * control pointreplaced by(v1 & 0x1f00) >> 4. The domain program takes its positions from those records, so a wrong address moves or drops the patches. The test draws it and checks each patch likeHullCode.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).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 (d39285d onmain8b3a6ba): 52/52 three times, and 52/52 under the layer with noVUID-line.maincontrol there:main8b3a6ba with only this PR's two test files. Both tests fail. The new lowered case is refused withnot affine in the control point and relative patch, and the GPU test stops onHullMaskedControlPoint's store at pc 0x60 with the same refusal. The two refusal cases pass on both.main9e568a3: the focused set passes 53/53, plain and under the layer, with noVUID-line. Themaincontrol (main's lowering with this PR's test files) fails the same two tests as on Windows, the GPU test at pc 0x60, also under the layer with noVUID-line:agc_driver_tessellation_testscreates its Vulkan instance before the refusal, so the comparison is 0 against 0.tools/check_conventions.py --base origin/mainreports no findings.Checklist
main; no other open PR implements the same functions. No open PR changes the address evaluator'smask.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; once fix(shader): read other control points' off-chip outputs in hull programs #9743 lands, this PR mergesmainand keeps both.