Repository navigation
fix(shader): index tessellation attributes at a constant address with constants - #8884
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 regression can miss the exact old index chain it is intended to reject. folded only records an integer op when both operand IDs are in constants. With the previous emitter, OpUMod(address, stride) can meet that condition, but the subsequent OpShiftRightLogical(UModResult, 4) and the component shift/and consume result IDs that are not added to constants or propagated through folded. The access-chain indices can therefore remain unmarked and the test may pass with the old emitter restored. Could the check evaluate/propagate constant-expression results (or inspect the expected access-chain index definitions recursively), and confirm that reverting only the emitter makes this assertion fail?
| const auto opcode = words[at] & 0xffffu; | ||
| if (opcode == spv::OpConstant && count >= 3u) constants.insert(words[at + 2u]); | ||
| const bool integer = opcode == spv::OpIAdd || opcode == spv::OpIMul || opcode == spv::OpUDiv || opcode == spv::OpUMod || opcode == spv::OpShiftRightLogical || opcode == spv::OpBitwiseAnd; | ||
| if (integer && count == 5u && constants.contains(words[at + 3u]) && constants.contains(words[at + 4u])) folded.insert(words[at + 2u]); |
There was a problem hiding this comment.
A small propagation change makes this catch chained expressions too:
| if (integer && count == 5u && constants.contains(words[at + 3u]) && constants.contains(words[at + 4u])) folded.insert(words[at + 2u]); | |
| if (integer && count == 5u && | |
| (constants.contains(words[at + 3u]) || folded.contains(words[at + 3u])) && | |
| (constants.contains(words[at + 4u]) || folded.contains(words[at + 4u]))) { | |
| folded.insert(words[at + 2u]); | |
| } |
Then the OpShiftRightLogical and OpBitwiseAnd results derived from the folded OpUMod become marked before the access chain is checked.
… constants TessellationPointer built the vertex, attribute and component indices of an attribute access from its address with OpUDiv, OpUMod, OpShiftRightLogical and OpBitwiseAnd even when the address was an immediate, and without SPIRV-Tools nothing folded them. AMD's Windows driver (32.0.21045.5002 on an RX 6700 XT, 32.0.21042.62 on an RDNA 2 iGPU) reads a per-vertex hull output indexed that way wrongly: a merged hull program's level read came back wrong, and agc_driver_tessellation drew a different wrong patch on each run. A read of the program's own record through a constant index failed the same way, and the test passes once the indices are OpConstants, as glslang emits them. The merged hull case now also checks that no access chain index of the hull stage is arithmetic on constants, so the shape is caught on every device. Fixes boykopovar#8861 Co-Authored-By: Claude <noreply@anthropic.com>
3f059e5 to
054f8ff
Compare
|
@MarcZX10 good catch, thank you. I applied your suggestion: an operand now counts if it is a constant or an already-folded result, so chained shifts and masks are marked too. The new head is On
On the RX 6700 XT:
|
MarcZX10
left a comment
There was a problem hiding this comment.
The updated regression now propagates folded integer expressions through chained shifts/masks and checks the access-chain indices, addressing the false-negative risk I raised. The author reports the test fails with only the emitter change reverted and passes with the fix on the RX 6700 XT; the targeted AMD before/after runs and validation-layer results are recorded in the PR. I reviewed the updated diff and found no remaining blocker. I could not reproduce the GPU path in this Linux environment.
|
A Linux run, since this PR's runtime CI jobs were skipped. It uses lavapipe from Mesa 25.2.8 with LLVM 20.1.2, the same driver as Linux CI, on Ubuntu 24.04 (WSL) with GCC 13.3, Release, This PR's commit |
|
Independent test on a second AMD driver and an RDNA 2 iGPU, the setup the PR lists as not tested. Setup: AMD Radeon 680M (RDNA 2 iGPU, Ryzen 7 6800H, deviceID 0x1681), AMD Windows driver 32.0.21030.2001 (Adrenalin 25.10.30.02, Vulkan 1.4.315), Windows 11 26200, WinLibs GCC 15.2.0 r7, Release.
Run 3 on
Khronos validation layer (SDK 1.4.363,
Result: the fix holds on a second AMD Windows driver version and on RDNA 2 integrated graphics. AI-assisted: yes |
|
Thanks for both follow-ups. The RX 6700 XT run with the emitter-only negative control confirms the new constant-index assertion catches the original path, and the Linux/lavapipe run extends coverage to the other CI driver. The independent Radeon 680M comparison is especially useful: three failures on the same |
|
An RDNA 3.5 data point: Radeon 860M (gfx1152, Windows 11 26300), AMD driver 32.0.13058.8001, MinGW GCC 15.2 Release.
So this driver and GPU don't show #8861: the merged hull check passes on On the PR head, the 14 tests matching |
|
NVIDIA, Windows. GeForce RTX 4080 SUPER, driver 616.92 (32.0.16.1692), Windows 11 Pro 26200, Ryzen 9 7950X3D, WinLibs GCC 15.2.0 r7, Release.
With both files at Khronos validation layer not run: the Vulkan SDK is not installed on this machine. AI-assisted: yes |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the fix and the thorough multi-GPU data. One difference from the dynamic path remains:
SpirvModuleEmitter.cpp:850: the dynamic path passes the control point throughinPatch(), which clamps withUMinfor bothControlInput(toinputControlPoints - 1) andEvaluationInput(tooutputControlPoints - 1). The immediate path clamps onlyControlInput, so an immediateEvaluationInputaddress at or beyondstride * outputControlPointsgives an out-of-range access-chain index wheremainclamped. Please clampEvaluationInputtooutputControlPoints - 1on the host as well, and add a case to the test with an immediate address past the last control point.
Everything else matches the dynamic path (LocalOutput/patch, own-lane kinds, % stride offsets).
|
Validated on AMD's Windows driver: Radeon RX 6700 XT, AMD proprietary driver 26.10.1, Windows 11 10.0.22621, WinLibs GCC 15.2 UCRT, Release.
|
|
The run on 32.0.21042.62 you asked for. AMD Radeon RDNA 2 iGPU (
Your 10 tests plus Not run: the Khronos validation layer (not installed on this machine). AI-assisted: yes (Claude Code); results above were run on this machine. |
…control point The dynamic path of TessellationPointer passes the control point through inPatch(), which clamps it with UMin to inputControlPoints - 1 for hull inputs and to outputControlPoints - 1 for domain inputs. The immediate path clamped only hull inputs, so a domain read at an immediate address past the last control point indexed the input array past its end. It now clamps domain inputs to outputControlPoints - 1 too. Co-Authored-By: Claude <noreply@anthropic.com>
|
@adriadam10 done in e4de56f. The immediate path now clamps an The new case is in RX 6700 XT, AMD driver 32.0.21045.16010, this head merged into
|
|
Re-ran
AI-assisted: yes (Claude Code); results above were run on this machine. |
|
Checked the new commit against the dynamic path: an immediate |
What
Fixes #8861.
TessellationPointer(SpirvModuleEmitter.cpp) computes the vertex, location and component indices of a tessellation attribute access from its address withOpUDiv,OpUMod,OpShiftRightLogicalandOpBitwiseAnd. It does this even when the address is an immediate. In the default build nothing folds them, since SPIRV-Tools is off, so the driver gets access chains whose indices are arithmetic on constants.AMD's Windows driver reads a per-vertex tessellation control output indexed that way wrongly. In
agc_driver_tessellation's merged hull program, invocation 0 reads control point 2's level from the per-vertex record that stands in for LDS. That read came back wrong, so a patch was missing or drawn flat, and which one changed from run to run.With an immediate address, the indices are now computed on the host and emitted as
OpConstants, which is the shape glslang emits. Dynamic addresses are unchanged. The host clamps the control point asinPatch()does on the dynamic path: a hull input toinputControlPoints - 1and, since e4de56f (from review), a domain input tooutputControlPoints - 1.How it was narrowed down on an RX 6700 XT, with local experiments that are not part of this PR:
mainds_read_b32at offset 72) replaced with a copy of its own registermain, so it is not cross-invocation visibilityWorkgroupmemory scope andAcquireRelease | WorkgroupMemory | UniformMemory | ImageMemoryinstead ofInvocationandNoneThe merged hull case also gets a new check: no access-chain index in its hull stage may be integer arithmetic on two constants. That catches this shape on every device, lavapipe included. On
mainthe check fails.Tested
Windows 11 26200, RX 6700 XT (gfx1031), AMD driver 32.0.21045.5002, MinGW GCC 15.2.0 Release,
ANYPS5_REQUIRE_VULKAN=1. Unless a line says otherwise, the runs below are of3f059e58conmainf47c85663;054f8ff4dthen added @MarcZX10's propagation to the new check, and e4de56f the domain-input clamp.agc_driver_tessellation, 3 runs per side from renamed copies of the executable, so the driver's pipeline cache starts empty:mainfails 3 of 3: level-1 patch not drawn once, level-5 patch without interior points twice.--repeat until-fail:3: 10 of 10 pass with this PR. The 10 areagc_driver_tessellation,agc_driver_prepared_graphics,agc_driver_provoking_vertex,agc_driver_rect_list_corner,agc_driver_pixel_barycentric_emulation,agc_driver_graphics,agc_driver_bda_device,agc_shader_memory,agc_tessellation_loweringandagc_shader_disk_cache.ctest -Von the same 10,maind0852defaagainst the same change ond0852defa, then again on this head:mainpasses 9 of 10 (agc_driver_tessellationfails); this PR passes 10 of 10.VUID-lines on either side.VK_LOADER_DEBUG=layershows the layer inserted in every GPU test process.the merged hull program indexes a tessellation attribute with arithmetic on constants instead of a constant.main6ef5a1c4c9, on AMD driver 32.0.21045.16010 (this PC's driver since 2026-10-21; the runs above used 32.0.21045.5002): a newagc_tessellation_loweringcase emits a domain read at the immediate address3 * 16, with 3 output control points and a 16-byte stride, and checks that the access chain indexes control point 2.failed: a domain read at an immediate address past the last control point reads the last control point.VUID-lines;VK_LOADER_DEBUG=layershows the layer inserted.outputs * controlStride(TessellationLowering.cpp:744-746).Other GPUs, from reviewers:
agc_driver_tessellationpasses 5 of 5 onmain2c4b116da9and on this head, so that driver does not show AGC: agc_driver_tessellation's merged hull program draws a different wrong patch on each run on AMD's Windows driver #8861; the 14 tests matchingtess|hull|spirv|recompiler|geometry|^agc_driver_graphics$pass on this head.maincecb540a6on an NVIDIA RTX 4080 SUPER (616.92, Windows 11 Pro 26200): the 10 tests pass 3 of 3. Withmain'sSpirvModuleEmitter.cppand this PR's tests,agc_driver_tessellationfails 3 of 3 with the new constant-index check's message, and with both files atmainit passes, so that driver does not show AGC: agc_driver_tessellation's merged hull program draws a different wrong patch on each run on AMD's Windows driver #8861's wrong patches either.054f8ff4don an RX 6700 XT with AMD driver 26.10.1 (Windows 11 22621), 10 runs per side:agc_driver_tessellationfails 10 of 10 onmaina6dcea927(the level-1 patch was not drawn as one flat triangle). It passes 10 of 10 with this PR and test(agc): read the sample order tests' depth through a sampled copy #9056 merged onto thatmain.054f8ff4dmerged intomaina6dcea927on 32.0.21042.62, the driver where they first saw the failure: an RDNA 2 iGPU (gfx1036), Windows 11, with an empty pipeline cache for each run.agc_driver_tessellationfails 3 of 3 onmainand passes 3 of 3 with this PR. An RTX 5070 Ti (617.42) passes 3 of 3 on both sides. Of 23 related tests on the PR side, onlyagc_driver_sample_order_grid_referencefails on the iGPU, and it fails onmaintoo (test(agc): read the sample order tests' depth through a sampled copy #9056).agc_driver_tessellationandagc_tessellation_loweringpass 3 of 3.Not tested:
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)🤖 Generated with Claude Code