Repository navigation
fix(agc): name the mesh limit a pipeline exceeds and its values - #11065
Conversation
The mesh pipeline check rejected a shader with one message for three limits, so a shader 4 bytes over the shared memory limit could not be told from one with too many output vertices. ValidateMeshLimits checks the workgroup, output vertices, output primitives and LDS size separately and names the limit and both values. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The refactor preserves the existing limit predicates and the added boundary tests cover each reported limit. I cannot approve yet because the repository requires the focused AGC tests on both main and this head with ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1 and VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, including comparison of VUID- diagnostics. The PR reports a local agc_driver_graphics_tests run, but not that main-versus-change validation comparison; hosted checks skip the AGC targets. Please report the test command, OS/GPU/driver, confirm the validation layer loaded in both logs, and note any new VUIDs. I have not reproduced a runtime defect in this diff.
|
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.
|
|
A Windows run of this head: Windows 11 Home 10.0.26200, WinLibs GCC 15.2.0 r7 (MinGW-w64 UCRT), Release.
AI-assisted: yes (Claude Code) |
klosixgaming-arch
left a comment
klosixgaming-arch
left a comment
There was a problem hiding this comment.
Reviewed head 708cc2260d by reading the diff and the existing review thread. I did not build or run this, so everything below is from the code, not from a test of mine.
What it does. Pulls the mesh-limit checks out of the Pipeline constructor into ValidateMeshLimits, with one Require per limit and a message that names the limit and both values. The conditions are the same four as before (invocations against both maxMeshWorkGroupInvocations and maxMeshWorkGroupSize[0], output vertices, output primitives, LDS bytes).
Checked each predicate against the old code: invocations are <= both limits as before, maxVertices/maxPrimitives are <=, and LDS is ldsSizeDwords * 4 widened to 64-bit before the multiply, same as the old expression. The new meshLimitTests hits both sides of every boundary with numbers that do not coincide (96/97, 256/257, 512/513, 3071/3072 dwords against 12284 bytes), so an off-by-one would show.
Useful for the open blockers: the old message was a bare "mesh workgroup exceeds device limits", which is hard to act on. The new one prints the pipeline's value and the device limit, which is what you need to tell whether a title is asking for something the device cannot do or whether the limit query is wrong.
Nits: the meshLimitTests fixture constructs MeshConfiguration with nine positional initialisers; a designated-field style would survive a reordering of that struct. The process requirement MarcZX10 raised (a main-versus-head run with validation layers) is real, and ycbuzi's Windows run in the thread covers the build; I'd still want that evidence recorded in the description.
Approving: it is a behaviour-preserving refactor with better diagnostics and boundary tests.
AI-assisted: yes. Written with AI assistance and reviewed by me.
|
I ran the focused mesh test on Command:
This change adds host-side checks and error text. It makes no Vulkan calls. AI-assisted: yes (Claude Code) |
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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.
|
Checked that the four predicates in |
What
The mesh pipeline check used one message for three limits. The message was
mesh output or LDS exceeds device limits. It did not show which limit failed. It did not show the values.This change moves the check to
ValidateMeshLimits. The function checks four limits separately: the workgroup size, the output vertices, the output primitives, and the LDS size. Each failure names the limit. It shows the value of the shader and the value of the device.Example:
mesh LDS 12288 bytes exceeds the device shared memory limit 12284.The conditions do not change. The same pipelines are accepted. The same pipelines are rejected. Only the text of the error changes.
A shader of PPSA21567 asked for 12288 bytes of LDS. MoltenVK reported a limit of 12284 bytes. The old message did not show this difference.
meshLimitTestsincore/libs/prx/libSceAgcDriver/tests/Graphics.cpptests the function. For each limit it checks one value over the limit and one value at the limit.Tested
macOS, Apple M2, x86-64 build under Rosetta 2, Release.
agc_driver_graphics_testspasses with the change.<=comparison of the LDS size was changed to<for one run. The test then fails withmesh LDS 12284 bytes exceeds the device shared memory limit 12284.python3 tools/check_conventions.py --base origin/mainpasses.maindoes not buildlibcon macOS, becauselink.his not found. The test ran in a separate tree that has local macOS patches and the same diff.The test sets the limits in a
Contextstructure. It does not create a Vulkan device. It did not run withVK_LAYER_KHRONOS_validation.Not tested: Linux, Windows, a draw on a GPU.
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