Repository navigation
fix(shader): answer image_get_resinfo on any T# dimension - #7550
adriadam10 merged 2 commits into
Conversation
|
The query-only path still appears to reject both multisample T# types. |
|
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.
|
|
I checked the overlap list. This PR doesn't depend on any of them and doesn't duplicate any of them:
AI-assisted: yes |
MarcZX10
left a comment
There was a problem hiding this comment.
The current head still omits both multisample T# shapes from the query-only 1D mode set. descriptorDimension decodes Color2DMsaa as Dim2DMsaa and Color2DMsaaArray as Dim2DMsaaArray, but the dimensionQueryOnly branch adds only Dim2D, Dim1DArray, Dim2DArray, and Dim3D (plus cube). decodeTableEntry therefore has no matching candidate for a DIM 1D image_get_resinfo on either MSAA descriptor. The new ImageResinfoDimension test covers cube, 3D, and 2D array descriptors, but not MSAA. Please include the 2D MSAA query mode and an array-MSAA mode/test if supported, or narrow the “any T# dimension” claim.
|
Apple GPU / MoltenVK run of Machine: Apple M2 Max, macOS 27.2, x86-64 build under Rosetta. Head de6d94d. Method: macos-port @ b7eb6b6 + ImportWatched + PR diff, not main.
With a local print added to the test, Apple returns the same answer for DIM 1D and for each T#'s own DIM: cube
GPUs tested: Apple M2 Max (MoltenVK 1.4.2 and MoltenVK main + SPIRV-Cross #2704) Written with AI assistance (Claude Code). I ran the builds and tests above on my machine. |
|
Thanks for the MoltenVK run and for documenting the existing baseline failures. The cube, 3D and 2D-array dimension-query results add useful coverage across both MoltenVK versions. My review still asks for the separate 2D MSAA and array-MSAA cases, since those descriptors decode to distinct runtime image modes. |
|
Hardware data for the DIM question, since the PR has no AMD run yet.
So on RDNA 2:
This is PC RDNA 2, not a PS5. I don't know which components PPSA01317's shader reads. If it only reads the size, this doesn't change what the title sees. |
|
Tested on AMD on Linux, which this PR had not covered.
Full
|
|
RTX 4090 data point on PPSA01317, continuing today's chain on D4697 and #6815 (same box, same stack): with this PR the Setup. Gran Turismo 7 PPSA01317 01.000.001 (base package), Windows 11 (26200), RTX 4090 (NVIDIA 616.92), AMD Ryzen 7 5800X, MinGW-w64 GCC 15.2.0 r7 (UCRT), Release Result. The control (the same tree without this PR, on #6815) aborts on
What the title does in that time, the same in all four runs: the "Sony Interactive Entertainment presents" splash (title-bar counter 18 FPS at 560 frames, +15 s), then a black window; by +55 s the counter is at about 1,320 frames and 12 FPS, and from there it advances by a handful of frames over the next minutes (run 2: 1,319 at +55 s, 1,322 at +245 s; run 3: 1,329 at +245 s; run 4: 1,331 at +170 s). It is not idle. A 15 s sampler on run 4 (
Chain on this box for a sampler on run 4 (15 s period; cpu is the process total, d15 the increase over the period)stderr of run 4 (warm, box idle) from the save-data mount to the abort, the per-shader
|
|
I compared #7550 at For composition, preserve I propose #6280 first, then rebase #7550 with both changes preserved; that is a proposal, not an agreed merge order. The separate DIM/layer-component and MSAA reviewer questions remain open, including thuanquang's desktop RDNA2 evidence; this composition note does not settle them or claim a combined execution result. AI-assisted: yes (Codex) |
|
Rebase handoff — rebased onto current Verified on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ninja, Release, To pick it up: Head is |
|
Correction: the first push didn't build (the PR's |
|
@blawesom @gjonn one thing in the handoff 227481d: the info encoder and decoder now write and read Heads-up on merge order: #7381 makes the same two edits, |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for this. A few things need fixing before it can merge:
- Layer count must follow the instruction's DIM. thuanquang measured
image_get_resinfoon RDNA 2 (comment above): width, height and levels come from the T#, but for a non-3D T# a DIM without layers (1D, 2D, 2D_MSAA) returns 0 in the third component (cube 8x8 →8, 8, 0, 1; 2D array 8x8x3 →8, 8, 0, 1); a 3D T# returns its depth for every DIM. The new view modes return the real layer count, andImageResinfoDimension.cpp:88(Check) requires DIM 1D to equal the T#'s own DIM, so the test pins the wrong value for cube and 2D array. Make the third component 0 when the instruction's DIM has no layers (except 3D T#), and change the test to expect that. - MSAA descriptors.
ResourceMaterializer.cpp:1523-1534adds no 2D MSAA / 2D MSAA array view, so DIM 1D on those T# still hits the incompatible-interface error (MarcZX10's review). HW answers them too (8, 8, 0, 1). Add the modes and a test row for each. - Rebase. The PR conflicts with
main.FormatVersion(ShaderDiskCache.hpp:16, 38) andfieldCount<ImageResource>()(ShaderDiskCache.cpp:143, 39) are stale:mainis at 40 and #7381 also takes 41. Use the next free values after rebasing, and re-runagc_shader_disk_cacheand the new test.
image_get_resinfo with DIM 1D on a cube, 3D or 2D-array T# was refused with "image address has too few coordinate components": the tracker made a 1D image that only the query uses, and no 1D mode matches such a T#. The returned size, layer count and levels come from the T#; DIM only sets the number of address registers. A 1D image used only by dimension queries now decodes the T#'s own dimension and accepts 2D, 1D array, 2D array, 3D and cube views. The flag is stored in the shader disk cache, so FormatVersion moves to 38. PPSA01317 queries a 64x64 cube with DIM 1D in a pixel shader. AI-assisted: yes
image_get_resinfo returns 0 in its third component when the instruction's DIM has no layers (1D, 2D, 2D MSAA) and the T# is not 3D, as measured on RDNA 2 in boykopovar#7550. Keep 1D-array T#s on their first layer and MSAA T#s refused, and record the unmeasured forms in the shader debt file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
de6d94d to
7b4b9d9
Compare
Dismissing a stale review: the requested changes were addressed after it and the merge queue blocks on it. Please request changes again if anything is still open.
What
image_get_resinfowith DIM 1D on a T# that is a cube, 3D or 2D array is refused onmain:The tracker turns the size query into a 1D image, and
ResourceMaterializerfinds no 1D mode that matches such a T#. But the size, layer count and level count thatimage_get_resinforeturns come from the T#. DIM only sets how many address registers the instruction reads.mainalready treats size queries on flat volumes this way.PPSA01317 does this in a pixel shader: it asks the size of a 64x64 cube T# (6 faces, 7 levels) with DIM 1D.
Change:
ImageResourcegetsdimensionQueryOnly. The tracker clears it when the image is used by any operation other thanImageQueryDimensions.RuntimeImageModesadds 2D, 1D array, 2D array, 3D and cube views for it, so the query reads the real view.FormatVersiongoes 37 -> 38, as earlier field additions did (for exampleflatVolumeLineCompatible).Possible conflicts:
FormatVersion; its body says 36 -> 37, andmainis now at 37. Whichever merges second takes the next value.image_get_resinforesults (levels past the mip chain) and touchesResourceMaterializer.cpp,ResourceTracker.cpp,Resources.hppandShaderDiskCache. A textual conflict is possible there, but the two changes do not overlap in behaviour.Tested
Windows 11, MinGW-w64 GCC, NVIDIA GeForce RTX 5090 Laptop GPU (driver 616.92). Tested on
mainat 5bf2e13. After that the branch was rebased cleanly onto 385ed71 (an unrelated LDS atomics merge) and not rebuilt.agc_driver_image_resinfo_dimension(tests/execution/ImageResinfoDimension.cpp). It runsimage_get_resinfoin a compute shader on cube, 3D and 2D-array T#s, once with DIM 1D and once with the T#'s own DIM, and requires both answers to match:main: fails withShaderRecompiler::Recompile: image address has too few coordinate components.image resinfo dimension tests passed).agc_shader_disk_cache(the image round-trip now setsdimensionQueryOnly = false),agc_shader_disk_cache_large_file,agc_shader_memoryandagc_driver_graphicspass.python tools/check_conventions.py --base origin/mainandpython tools/check_test_registration.pypass.Not tested: AMD, Intel, Linux, macOS. The title was not run on this exact branch.
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)