Repository navigation
Conversation
|
@yinx080 thanks. Two independent reviews agree the approach is right: the footprint order matches
Please keep the non-zero-LOD control that fails on |
|
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.
|
fbe13f0 to
8502ef1
Compare
|
Thanks for the careful review. Pushed a revision (rebased on f7e84c4); all four points are addressed:
Results (Windows 11 Pro 10.0.26200, NVIDIA GeForce RTX 3060 Laptop GPU, driver 592.82;
Debt entry: LOD_BIAS is modelled now; a T# Mirror-once is not in the test: The overlap bot lists #9183, which also changes |
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — approved
The PR merges cleanly onto main and builds. The new ImageGatherLod test passes with the validation layer loaded and no VUID- lines, and it fails on main's production code with the error the description quotes. The neighbouring gather and sample tests pass on both trees. I found nothing blocking. Two small follow-ups are below.
What I ran
- PR head:
8502ef15f7557f7972c066f5b24e07ee6e5ee9ba main:ba5b5330b37724bca4b99f30faab504653635a04- Tree under test:
mainwith the head merged in using--no-ff(no conflicts), in a clean worktree. - Machine: Arch Linux 7.2.11-zen, GCC 16.2.1, Intel Core i9-14900KF, NVIDIA GeForce RTX 4070 Ti SUPER (driver 615.78.08), Release build with Ninja.
git fetch origin main pull/12790/head:refs/review/12790
git checkout --detach origin/main
git merge --no-ff --no-edit refs/review/12790
git diff --check origin/main...refs/review/12790
python3 tools/check_conventions.py --base origin/main
cmake -S . -B build
cmake --build build -j10 --target \
agc_driver_image_gather_lod_tests agc_driver_image_gather_one_dimensional_tests \
agc_driver_image_gather_one_dimensional_array_tests agc_driver_image_packed_horizontal_gather_tests \
agc_driver_image_sample_lod_clamp_tests agc_driver_image_sample_unnormalized_tests \
agc_driver_image_sample_shared_sampler_tests agc_driver_image_sample_border_color_tests \
image_sample_gather_tests agc_driver_image_texel_status_tests agc_driver_image_converted_unorm_tests \
agc_driver_image_srgb_load_tests agc_driver_image_sint8_tests agc_driver_image_cache_bits_tests
VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 \
VK_LOADER_DEBUG=error,warn,layer ctest --test-dir build -j4 -V --output-on-failure -R \
'^(agc_driver_image_gather_lod|agc_driver_image_gather_one_dimensional|agc_driver_image_gather_one_dimensional_array|agc_driver_image_packed_horizontal_gather|agc_driver_image_sample_border_color|agc_driver_image_sample_lod_clamp|agc_driver_image_sample_shared_sampler|agc_driver_image_sample_unnormalized|image_sample_gather|agc_driver_image_texel_status|agc_driver_image_converted_unorm|agc_driver_image_srgb_load|agc_driver_image_sint8|agc_driver_image_cache_bits)$'
# baseline / negative control: the same build and ctest command after
git checkout origin/main -- <the five changed files under core/shader>The changed test file only adds a new test, so the negative-control tree is main's production code with the new test added. It was also the main baseline for the other 13 tests.
Results
| Check | Result |
|---|---|
Merge onto main |
Clean (docs/dev/debt/shader.md auto-merged) |
git diff --check |
Clean |
check_conventions.py --base origin/main |
0 errors, 1 warning (debt-length, see Non-blocking) |
| Build of the 14 test targets (merged tree) | OK |
| Focused tests, merged tree, validation layer | 14/14 passed |
Focused tests, main production code + new test |
13/14; agc_driver_image_gather_lod fails, the rest pass |
VUID- lines |
0 on the merged tree, 0 on main |
was not found but was requested by env var VK_INSTANCE_LAYERS |
Absent from both logs; the loader logged inserting VK_LAYER_KHRONOS_validation 13 times (merged) and 14 times (main) |
| Test duration | agc_driver_image_gather_lod 0.44–0.72 s |
Merged tree
1164: image 2D explicit-LOD gather tests passed
3/14 Test #1164: agc_driver_image_gather_lod ..................... Passed 0.72 sec
...
100% tests passed out of 14
main production code with the new test
1164: AGC graphics: clamp to edge, point: thread 1 (LOD 1.000000, level 1) component 0 is 108, expected 113
1/14 Test #1164: agc_driver_image_gather_lod .....................***Failed 0.70 sec
...
93% tests passed, 1 tests failed out of 14
check_conventions.py
docs/dev/debt/shader.md:213: warning: [debt-length] Technical debt entry over 1000 characters: state the gap first (what differs from the console, what is unverified, what throws) and keep only the behaviour it needs: 1186 characters
1 findings, 0 errors
Negative control
I restored Resources.hpp, DescriptorBindingBuilder.cpp, ShaderInfoCollector.cpp, PipelineSpecialization.hpp and SpirvImageEmitter.cpp from origin/main and kept the new test. agc_driver_image_gather_lod then failed on its first non-zero LOD: thread 1, LOD 1, read 108 (mip 0) where 113 (mip 1) was expected. The author reports the same message. After I put the merged files back, the test passed again.
Findings
Blocking
None.
Non-blocking
-
The debt entry is inaccurate about 1D images and leaves out 2D arrays.
docs/dev/debt/shader.md:213(line 212 on the head) says thatimage_gather4_l"on 1D, 3D or cube images" still gathers the base level. Two parts of that do not match the code:- On 1D, 1D-array and remapped-depth-1D images,
image_gather4_ldoes not fall back to a base-level gather. It is refused:SpirvImageEmitter.cpp:1437-1439callsctx.Fail(... "has an unsupported 1D gather variant")wheneverRdnaImageSampleFlagLevelZerois not set. - The new path's guard at
SpirvImageEmitter.cpp:1481requiresdimension == RdnaImageDimension::Dim2D. Soimage_gather4_lon a 2D array also keeps the base-level gather, and the entry does not mention that case.
This matters because a reader of the debt file would expect 1D titles to render wrong rather than abort, and would not look for 2D-array HZB reads. Suggested fix: write "on 2D array, 3D or cube images", and say that 1D explicit-LOD gathers throw. Shortening the entry at the same time would also clear the
debt-lengthwarning (1186 characters against a limit of 1000). - On 1D, 1D-array and remapped-depth-1D images,
-
The graphics-pipeline-library prebuild specializes on an all-zero sampler. This is a question about the prebuild path; I did not measure it.
RegistrationResultgives up on shaders whose specialization depends on sampler words (FoldsTexelOffsets), and otherwise materializes the shader with zeroed sampler descriptors. The new block atDescriptorBindingBuilder.cpp:316is not gated on that path. For a fragment shader that usesimage_gather4_l, the prebuild therefore getsGatherLodBase= wrap,MAX_LOD0 andMIP_FILTERnone. With specialization present,Recompiler.cpp:1232builds a specialized module rather than the registration module. A draw with a real sampler therefore gets a different module, so the prebuild looks wasted rather than wrong. If that is the case, addingSamplerUseGatherLodsamplers to the early return next toFoldsTexelOffsetswould avoid the useless prebuild. This only applies wheregraphicsPipelineLibraryis enabled.
Other things I checked and found correct:
- The S# fields are extracted from the right bits:
MIN_LOD/MAX_LODfrom word 1 [23:0] as u4.8,CLAMP_X/CLAMP_Yfrom word 0 [5:0],LOD_BIASfrom word 2 [13:0] as s5.8,MIP_FILTERfrom word 2 [27:26]. - The specialization defaults (
4 << 24, which reads as unsupported) mean that a sampler-table sampler, or a sampler that never got the constant, keeps the old gather. GatherAddressmatches the address-mode mapping inGuestSamplerResource.cpp, and Vulkan's integer wrap, mirrored-repeat and mirror-clamp-to-edge formulas.GatherLodBase+ 32 =GatherLodBiasBase, and the bias range also has 32 IDs. Both fitShaderInfo::MaxSamplers= 32 without overlapping another range.- Sampler words are fully part of the binding-plan key (
preparedBindingPlan) and of the result-memo key, so a sampler with a different LOD state cannot reuse a stale specialization. - With an unnormalized sampler, the gather is still refused through
UnnormalizedUseReason, becauseSamplerUseGatheris the lower bit.
The maintainer's four points from the earlier comment are covered by the test cases (fractional and tie LODs, bilinear, MIN/MAX_LOD, MIP_FILTER none, a large/+inf/NaN LOD, UINT with a bilinear sampler), and all of them pass on this NVIDIA host.
Not verified
- I ran nothing on AMD or Windows. The behaviour the debt entry lists as unmeasured on hardware (tie rounding, linear
MIP_FILTER,LOD_BIAS_SEC, T#MIN_LOD) was not checked on a console. - I did not run PPSA23276, so the in-game result is the author's.
- I did not exercise
image_gather4_lfrom a pixel shader, or the graphics-pipeline-library path (finding 2 comes from reading the code). - I did not check the textual overlap with #9183 beyond confirming that it is a different feature (3D
image_gather4_lz).
|
@yinx080 thanks, this round answers all four points: fetching the footprint at a level chosen in the shader removes the sampler's clamp and filter from the result, the LOD is clamped in floating point before any conversion, the integer-format case needs no sampler at all, and the new variants (fractional and tie LODs, NaN/inf/huge, bilinear, MIN/MAX_LOD, bias, MIP_FILTER none, UINT with a bilinear sampler) pin it. One case the switch to
Smaller points, not blocking:
|
✅ NVIDIA GPU test: passedPR head
Intermittent validation messages (1, seen once in the parallel run, not reproduced when the test was re-run alone 2 times; not counted against this PR)Failing on main too (5, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (4 IDs in 407 tests)Skipped (39)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
8502ef1 to
813217f
Compare
|
Thanks. Pushed a third commit (rebased on 899ae3d):
Smaller points:
Results (same machine and settings as before,
The |
XairRrR
left a comment
There was a problem hiding this comment.
Re-checked — changes still requested
The new commit 813217f0b5 raises the fetched level to the T# MIN_LOD, keeps table images and samplers on the base-level gather at build time, and adds three test variants. One of those variants makes agc_driver_image_gather_lod log three new VUID- validation errors. The production code that raises them is already on main, but AGENTS.md asks that a GPU change add no new VUID- errors.
What changed since my last review
- The branch was rebased onto
899ae3d1a.git range-diffshows the two commits I reviewed are unchanged (5f2c39f214→f68dbf0702,8502ef15f7→9d29fb18fe). - New commit
813217f0b5"fix(shader): keep an explicit-LOD gather's fetch at the view's minimum LOD". It does five things:- adds the
GatherImageMinLodBasespecialization: the T#MIN_LODaboveBASE_LEVEL, set per image inDescriptorBindingBuilder::Prepare; - applies the S# clamp in a fixed order,
NMin(NMax(λ, MIN_LOD), MAX_LOD), and then raises λ to the image's minimum LOD; - gates the new path on
table == NoTablefor both the image and the sampler; - adds three test variants: T#
MIN_LOD2.5, mirror-once (modes 3 and 7), and the border fallback (modes 6 and 4); - rewrites the debt entry.
- adds the
- The author's 14:54 UTC comment replies to oneandonlydean's T#
MIN_LODpoint and reports results, including the newVUID-lines from the mirror-once variant.
What I ran
- PR head:
813217f0b543b4a61f5449ed66e0bbea444b40b1 main:bb04a4f1ea0175e9e4f149f0359a764d62ff3296- Tree under test:
mainwith the head merged in using--no-ff(no conflicts), in a clean worktree. - Machine: Arch Linux 7.2.11-zen, GCC 16.2.1, Intel Core i9-14900KF, NVIDIA GeForce RTX 4070 Ti SUPER (driver 615.78.08), Release build with Ninja.
git fetch origin main pull/12790/head:refs/review/12790
git range-diff ba5b5330b3..8502ef15f7 899ae3d1ad..refs/review/12790
git checkout --detach origin/main
git merge --no-ff --no-edit refs/review/12790
git diff --check origin/main...refs/review/12790
python3 tools/check_conventions.py --base origin/main
cmake -S . -B build
cmake --build build -j10 --target \
agc_driver_image_gather_lod_tests agc_driver_image_gather_one_dimensional_tests \
agc_driver_image_gather_one_dimensional_array_tests agc_driver_image_packed_horizontal_gather_tests \
agc_driver_image_sample_lod_clamp_tests agc_driver_image_sample_unnormalized_tests \
agc_driver_image_sample_shared_sampler_tests agc_driver_image_sample_border_color_tests \
image_sample_gather_tests agc_driver_image_texel_status_tests agc_driver_image_table_tests
VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 \
VK_LOADER_DEBUG=error,warn,layer ctest --test-dir build -j4 -V --output-on-failure -R \
'^(agc_driver_image_gather_lod|agc_driver_image_gather_one_dimensional|agc_driver_image_gather_one_dimensional_array|agc_driver_image_packed_horizontal_gather|agc_driver_image_sample_border_color|agc_driver_image_sample_lod_clamp|agc_driver_image_sample_shared_sampler|agc_driver_image_sample_unnormalized|image_sample_gather|agc_driver_image_texel_status|agc_driver_image_table)$'
# main baseline and negative control: the same build and ctest after
git checkout origin/main -- <the five changed files under core/shader>
# probes: one extra Run(...) line inserted temporarily into ImageGatherLod.cpp,
# built and run with the same environment, on both treesResults
| Check | Result |
|---|---|
git range-diff against the previously reviewed commits |
The first two commits are identical; one new commit |
Merge onto main |
Clean |
git diff --check |
Clean |
check_conventions.py --base origin/main |
All checks passed. The debt-length warning from my last review is gone |
Build of the 11 targets, merged tree and main production code |
OK |
| Focused tests, merged tree, validation layer | 11/11 passed, including all 12 ImageGatherLod variants (0.50–0.65 s) |
Focused tests, main production code + the PR's test |
10/11; only agc_driver_image_gather_lod fails, with the message the author quotes |
VUID- IDs, merged tree |
3 new: VUID-VkSamplerCreateInfo-addressModeU-01079, -addressModeU-parameter, -addressModeV-parameter, all from agc_driver_image_gather_lod |
VUID- IDs, main |
None |
was not found but was requested by env var VK_INSTANCE_LAYERS |
Absent from both logs; the loader inserted VK_LAYER_KHRONOS_validation 10 times (merged) and 11 times (main) |
agc_driver_image_table |
Passed on both trees with no SYNC- lines on this host. The author reports a SYNC-HAZARD failure on both trees on their machine |
Merged tree: the new validation errors
1172: Validation Error: [ VUID-VkSamplerCreateInfo-addressModeU-01079 ] | MessageID = 0x1aa95c2b
1172: The Vulkan spec states: If the samplerMirrorClampToEdge feature is not enabled, and if the VK_KHR_sampler_mirror_clamp_to_edge extension is not enabled, addressModeU, addressModeV and addressModeW must not be VK_SAMPLER_ADDRESS_MODE_MIRROR_CLAMP_TO_EDGE
1172: Validation Error: [ VUID-VkSamplerCreateInfo-addressModeU-parameter ] | MessageID = 0x3fcec07
1172: Validation Error: [ VUID-VkSamplerCreateInfo-addressModeV-parameter ] | MessageID = 0xf8361085
1172: image 2D explicit-LOD gather tests passed
10/11 Test #1172: agc_driver_image_gather_lod ..................... Passed 0.65 sec
100% tests passed out of 11
main production code with the PR's test
1172: AGC graphics: clamp to edge, point: thread 1 (LOD 1.000000, level 1) component 0 is 108, expected 113
1/11 Test #1172: agc_driver_image_gather_lod .....................***Failed 1.02 sec
91% tests passed, 1 tests failed out of 11
Previous findings
-
Debt entry: 1D images and 2D arrays. Partly resolved, still open.
- Resolved: the entry was rewritten and no longer trips
debt-length. - Still open: it still says 1D images "still gather the base level". On 1D, 1D-array and remapped-depth-1D images,
image_gather4_lis refused atSpirvImageEmitter.cpp:1444-1446(ctx.Fail(... "has an unsupported 1D gather variant")). - Still open: 2D arrays also keep the base-level gather because of the
Dim2Dguard, and the fallback list does not name them.
Non-blocking.
- Resolved: the entry was rewritten and no longer trips
-
The graphics-pipeline-library prebuild specializes on an all-zero sampler. Still open and unanswered; the code is unchanged. The early return at
Recompiler.cpp:2070still covers onlyFoldsTexelOffsets. The new imageMIN_LODconstant takes 0 from zeroed T# words, so the new commit adds nothing to this. Non-blocking question.
New findings
Blocking
- The mirror-once variant adds three new
VUID-validation errors. The variant is atImageGatherLod.cpp:234.- What I see. I confirm the author's report: on the merged tree,
agc_driver_image_gather_lodlogsVUID-VkSamplerCreateInfo-addressModeU-01079,-addressModeU-parameterand-addressModeV-parameter.main's log has none of them. - Where they come from. I moved a mirror-once variant to the front of the test and ran it against
main's production code, and the same three IDs appeared. So they come from code that is already onmain, not from the shader change.GuestSamplerResource.cpp:37-41maps clamp modes 3, 5 and 7 toVK_SAMPLER_ADDRESS_MODE_MIRROR_CLAMP_TO_EDGE, and nothing undercore/enablessamplerMirrorClampToEdge. - Why it blocks. AGENTS.md asks that an AGC or shader recompiler change add no new
VUID-errors. Once this merges,agc_driver_image_gather_lodreportsPassedwhile it logs invalid sampler creation on every run. - Suggested fix. Either option works:
- Land the sampler fix the author mentions first, and list it under
Depends on. That fix would enablesamplerMirrorClampToEdgeat device creation when it is supported, and refuse or emulate the mode when it is not. - Drop the mirror-once variant here and add it with that fix.
- Land the sampler fix the author mentions first, and list it under
- What I see. I confirm the author's report: on the merged tree,
Non-blocking
- With
MIP_FILTERnone, the fetch ignores the T#MIN_LOD.- The code.
SpirvImageEmitter.cpp:823-831raisesnearestto the image's minimum LOD, butleveltakes the constant 0 whenmipNoneis set. - Why it matters. When the T#
MIN_LODis at least 1 aboveBASE_LEVEL, the view gets aminLodthroughChainMinLod. The fourOpImageFetches then read belowminLodInteger. The Vulkan 1.4.330 spec says such values are zero withrobustImageAccess2and undefined otherwise. - Probe. I added
{MipNone, T# MIN_LOD 640}, theT# MIN_LOD 2.5variant withMIP_FILTERnone. On the merged tree, thread 0 read 0 where any mip level holds a non-zero texel:component 0 is 0, expected 108. - Not a regression. The same probe on
mainalso reads 0. The gather section of the spec makesOpImageGatherbelowminLodIntegerundefined as well, so the view does not clamp the old base-level gather either. That also qualifies the premise of oneandonlydean's point 1. - Why it is still worth fixing. The new commit's message says "the fetched level never falls below the view's integer minimum LOD". That is not true for this branch. The test's reference model encodes the same gap:
Level()returns 0 forMipNonebefore it looks attextureMinLodRaw(ImageGatherLod.cpp:150). - Suggested fix. For
mipNone, usefloorof the image's minimum LOD, clamped to the last level, rather than 0. Add that variant to the test, and name the case as unmeasured in the debt entry. If the console's behaviour forMIP_FILTERnone with a T#MIN_LODis unknown, the debt entry is the place to say so.
- The code.
Other things I checked in the new commit and found correct:
- The constant matches the view's minimum LOD.
EffectiveMinLodgives the view an absoluteMIN_LOD, or none whenMIN_LOD <= BASE_LEVEL·256. The constant is the same value rebased to the view (MIN_LOD - BASE_LEVEL·256, else 0), andOpImageFetch'sLodis view-relative.- So for mip filters other than none, the fetched level
BASE_LEVEL + nearestis at least the view'sminLodInteger, becauseceil(λ + 0.5) - 1 >= floor(λ). EffectiveMinLod's cap atlastLevelmatches the shader's clamp tolevels - 1.
GatherImageMinLodBase+ 64 (ShaderInfo::MaxImages) does not overlap another specialization range. The dedupe on equal constants is safe because only this loop writes those IDs, and each value depends on the image alone.- The image constant is set only for non-table images, and the shader now also takes the base-level path for table images and table samplers, so the default constant is never relied on there.
Not verified
- Nothing ran on AMD or Windows, and I did not run PPSA23276. The in-game result in the author's comment is author-reported.
- The hardware behaviour of the T#
MIN_LODclamp and of the mirror-once modes is unmeasured, as the debt entry says. - I did not reproduce the author's
agc_driver_image_tableSYNC-HAZARD: the test passed on both trees here. - I did not exercise the graphics-pipeline-library prebuild path.
To get this to a good state
- Remove the three new
VUID-errors: land thesamplerMirrorClampToEdgefix first and list it underDepends on, or drop the mirror-once variant from this PR. - Optionally, apply the T#
MIN_LODto theMIP_FILTERnone branch as well, and add a variant for it. - Optionally, correct the debt entry for 1D (it throws) and 2D arrays (base-level gather).
|
Thanks. Pushed a fourth commit:
Results (same machine and settings as before,
|
XairRrR
left a comment
There was a problem hiding this comment.
Re-checked — approved
The new commit 01b2319b7d drops the mirror-once variant, fetches MIP_FILTER none at the floor of the T# MIN_LOD, keeps gather-LOD shaders out of the pipeline-library prebuild and corrects the debt entry. The focused tests now pass on the merged tree with no VUID- lines.
What changed since my last review
- One new commit,
01b2319b7d"fix(shader): fetch an explicit-LOD gather with MIP_FILTER none at the view's minimum level". It changes 4 files, +9/−7:SpirvImageEmitter.cpp: forMIP_FILTERnone the level is nowfloor(imageMinLod), clamped to[0, levels - 1], instead of 0.Recompiler.cpp:RegistrationResultreturnsnullptrfor a shader with aSamplerUseGatherLodsampler, as well as forFoldsTexelOffsets.ImageGatherLod.cpp: removes the mirror-once variant, addsMIP_FILTER none, T# MIN_LOD 2.5, and updates the referenceLevel()to match.docs/dev/debt/shader.md: rewrites the entry.
- The first three commits are unchanged.
- The author's 15:29 UTC comment answers all four points from my last review.
What I ran
- PR head:
01b2319b7dc07947ed5bf1a3005e31da622d0410 main:bb04a4f1ea0175e9e4f149f0359a764d62ff3296, the samemainas last time.- Tree under test:
mainwith the head merged in using--no-ff(no conflicts), in a clean worktree. - Machine: Arch Linux 7.2.11-zen, GCC 16.2.1, Intel Core i9-14900KF, NVIDIA GeForce RTX 4070 Ti SUPER (driver 615.78.08), Release build with Ninja.
git fetch origin main pull/12790/head:refs/review/12790
git diff 813217f0b543 refs/review/12790
git checkout --detach origin/main
git merge --no-ff --no-edit refs/review/12790
git diff --check origin/main...refs/review/12790
python3 tools/check_conventions.py --base origin/main
cmake --build build -j10 --target \
agc_driver_image_gather_lod_tests agc_driver_image_gather_one_dimensional_tests \
agc_driver_image_gather_one_dimensional_array_tests agc_driver_image_packed_horizontal_gather_tests \
agc_driver_image_sample_lod_clamp_tests agc_driver_image_sample_unnormalized_tests \
agc_driver_image_sample_shared_sampler_tests agc_driver_image_sample_border_color_tests \
image_sample_gather_tests agc_driver_image_texel_status_tests agc_driver_image_table_tests
VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 \
VK_LOADER_DEBUG=error,warn,layer ctest --test-dir build -j4 -V --output-on-failure -R \
'^(agc_driver_image_gather_lod|agc_driver_image_gather_one_dimensional|agc_driver_image_gather_one_dimensional_array|agc_driver_image_packed_horizontal_gather|agc_driver_image_sample_border_color|agc_driver_image_sample_lod_clamp|agc_driver_image_sample_shared_sampler|agc_driver_image_sample_unnormalized|image_sample_gather|agc_driver_image_texel_status|agc_driver_image_table)$'
cmake --build build -j10 --target agc_driver_recorder_tests
ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -j4 --output-on-failure -R '^agc_pipeline_library'
# negative control: the previous commit's emitter with the new test
git checkout 813217f0b543 -- core/shader/recompiler/SpirvBackend/src/SpirvImageEmitter.cpp
# build agc_driver_image_gather_lod_tests, run it with the same environment
# main baseline: main's production code with the PR's test
git checkout origin/main -- core/shader
# same build and validation ctest as above, then the merged files were restoredResults
| Check | Result |
|---|---|
Merge onto main |
Clean |
git diff --check |
Clean |
check_conventions.py --base origin/main |
All conventions checks passed |
Build: the 11 focused targets and agc_driver_recorder_tests |
OK |
| Focused tests, merged tree, validation layer | 11/11 passed. agc_driver_image_gather_lod passed in 1.30 s and printed image 2D explicit-LOD gather tests passed |
agc_pipeline_library* tests, merged tree |
6/6 passed |
VUID- IDs, merged tree |
None |
VUID- IDs, main production code |
None |
was not found but was requested by env var VK_INSTANCE_LAYERS |
Absent from both logs. The loader inserted VK_LAYER_KHRONOS_validation 10 times (merged) and 11 times (main) |
main production code + the PR's test |
10/11. Only agc_driver_image_gather_lod fails: clamp to edge, point: thread 1 (LOD 1.000000, level 1) component 0 is 108, expected 113 |
agc_driver_image_table |
Passed on both trees, with no SYNC- lines on this host |
Negative control
I restored SpirvImageEmitter.cpp from 813217f0b5, the commit I reviewed last time, and kept the new test. The new variant fails:
Output
1/1 Test #1172: agc_driver_image_gather_lod ......***Failed 0.84 sec
AGC graphics: MIP_FILTER none, T# MIN_LOD 2.5: thread 0 (LOD 0.000000, level 2) component 0 is 0, expected 146
With all of main's production code, the test fails on its first variant, as it did last time.
Previous findings
- New
VUID-errors from the mirror-once variant (blocking): Resolved. The variant and its two constants are gone fromImageGatherLod.cpp. The merged-tree log has noVUID-lines, and neither hasmain's. The author plans to add the variant back with thesamplerMirrorClampToEdgefix. MIP_FILTERnone ignored the T#MIN_LOD: Resolved. SeeSpirvImageEmitter.cpp:831-834.imageMinLodis the view-relative constant,MIN_LOD - BASE_LEVEL·256, or 0.BASE_LEVEL + floor(imageMinLod), capped at the last level, equals the view'sminLodInteger, so the fetch no longer reads below it.- The reference model at
ImageGatherLod.cpp:149matches, because every variant usesBASE_LEVEL0. - The new variant at line 233 covers the change; see the negative control.
- Debt entry for 1D and 2D arrays: Resolved.
shader.md:214now says thatimage_gather4_lthrows on 1D images. It lists 2D arrays among the base-level gathers, and namesMIP_FILTERnone with a T#MIN_LODas unverified. - Pipeline-library prebuild specialized on an all-zero sampler: Resolved.
Recompiler.cpp:2070now returnsnullptrfor any sampler withSamplerUseGatherLod, so those shaders get no zero-sampler registration module. The sixagc_pipeline_library*tests pass. None of them contains animage_gather4_lshader, so the new condition itself is not exercised.
New findings
Blocking
None.
Non-blocking
-
Parts of the PR description no longer match the code. Each of these could be updated:
Whatstill says "base level forMIP_FILTERnone".- The test list still names "mirror once (3, 7)" and not the new
MIP_FILTER none, T# MIN_LOD 2.5variant. Testedstill describes the mirror-onceVkSamplerCreateInfoIDs.
The 15:29 UTC comment does have the current results.
Not verified
- Nothing ran on AMD or Windows, and I did not run PPSA23276. The author's in-game and Windows results are author-reported.
- The hardware behaviour of
MIP_FILTERnone combined with a T#MIN_LODis unmeasured. The debt entry says so. - No test runs an
image_gather4_lshader through the pipeline-library registration path. - I did not reproduce the author's
agc_driver_image_tableSYNC-HAZARD: the test passed on both trees here.
✅ NVIDIA GPU test: passedPR head
Failing on main too (5, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (4 IDs in 401 tests)Skipped (40)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
An AMD RDNA 4 run, since the description lists an RTX 3060 Laptop GPU only. Windows 11 Pro 10.0.22631, AMD Radeon RX 9070 XT (
AI-assisted: yes (Claude Code) |
jotitadev
left a comment
There was a problem hiding this comment.
Linux lavapipe run of head 01b2319b7d: agc_driver_image_gather_lod fails on any device without VK_EXT_image_view_min_lod, and lavapipe (CI's linux device) is one of them. The linux job is skipped for this head, and the runs posted so far are on NVIDIA and AMD, so no report shows it yet.
Setup: i7-8700, Ubuntu 26.04, GCC 15.2, Release, lavapipe (Mesa 26.0.3, LLVM 21.1.8), ANYPS5_NO_SHADER_CACHE=1. Runs on the head alone and on the head merged --no-ff onto main 2a1ae91175, 3 runs each:
AGC graphics: guest texture descriptor uses a minimum LOD clamp, which needs VK_EXT_image_view_min_lod
Cause. The two T# MIN_LOD 2.5 variants put a T# minimum LOD on the image. Without the extension, main's texture path refuses that descriptor (Texture.cpp, Require(context.imageViewMinLod, ...)) before the gather runs. The test has no skip for that, so the whole test fails on the first of the two variants. MoltenVK has no VK_EXT_image_view_min_lod either.
Fix I checked: run the two variants only when the device has the extension, through the same test accessor #12662 uses:
+namespace AgcDriver {
+
+struct VulkanDeviceTestAccess {
+ static bool ImageViewMinLod(const VulkanDevice& device) { return device.graphicsContext().imageViewMinLod; }
+};
+
+}
+
@@ int main() {
- Run(*device, {Format8888UNorm, ClampEdge, ClampEdge, FilterPoint, MipPoint, 0u, AllLevels, 0, 640u, "T# MIN_LOD 2.5"});
- Run(*device, {Format8888UNorm, ClampEdge, ClampEdge, FilterPoint, MipNone, 0u, AllLevels, 0, 640u, "MIP_FILTER none, T# MIN_LOD 2.5"});
+ if (AgcDriver::VulkanDeviceTestAccess::ImageViewMinLod(*device)) {
+ Run(*device, {Format8888UNorm, ClampEdge, ClampEdge, FilterPoint, MipPoint, 0u, AllLevels, 0, 640u, "T# MIN_LOD 2.5"});
+ Run(*device, {Format8888UNorm, ClampEdge, ClampEdge, FilterPoint, MipNone, 0u, AllLevels, 0, 640u, "MIP_FILTER none, T# MIN_LOD 2.5"});
+ } else {
+ std::puts("T# MIN_LOD cases skipped, the device has no VK_EXT_image_view_min_lod");
+ }With it, agc_driver_image_gather_lod passes 3/3 on lavapipe, on the head alone and merged onto main. The other variants, including border modes, still run there. Devices that have the extension run the same cases as before.
Also: this PR and #12662 conflict in PipelineSpecialization.hpp. #12662 also rejects gathers at a T# clamp of one level or more in its shader lane, which is the lane MoltenVK uses. I haven't checked how these two variants behave with both PRs applied.
GPUs tested: lavapipe (Mesa 26.0.3, Linux).
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Linux / AMD RDNA 4 (RADV) run of head Host & GPU: CachyOS Linux ( Environment:
AI-assisted hardware test run (Antigravity). |
|
Thanks, all, and thanks RNK123 for the RADV run. Pushed a fifth commit with the change jotitadev checked: the two I also updated the description, as XairRrR suggested: Results on the RTX 3060 Laptop (same settings as before): your 11 focused tests are 10/11, with all twelve On #12662 and #12980, which builds on it: I haven't tried them with this PR. The |
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (5 IDs in 480 tests)Skipped (44)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
CnRJay
left a comment
There was a problem hiding this comment.
The semantics are carefully argued. The part I would want measured is the cost on the shader side:
- More variants. Samplers used by
image_gather4_lare now specialized on their S# LOD state (MIN_LOD,MAX_LOD,LOD_BIAS,MIP_FILTER,CLAMP_X/CLAMP_Y) and images on the T#MIN_LOD. Each distinct sampler or T# state used by a gather becomes its own pipeline/shader variant. For UE5 Nanite culling, which gathers the HZB at many mip levels with whichever samplers the title binds, how many extra variants does PPSA23276 compile per frame, and what is the effect on shader-cache size and first-hit compile time? Please attach a count from a run with the change againstmain. - Larger SPIR-V on every gather. The path replaces one
OpImageGatherwith a LOD computation, four clamped coordinate computations and fourOpImageFetchplus the wrap-mode handling. For a culling shader that issues four gathers per instance, that is a lot more ALU per invocation than the single hardware gather. Do you have the dispatch time of the Nanite culling pass before and after, fromtools/trace_digest.pyon the same scene? - Images or samplers from a table, and border modes 4 and 6, keep the base-level gather, so the result stays wrong for those. Is that recorded in the debt file as a known divergence?
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
This pull request does not merge cleanly into main (c955a63): head 3a6f213 conflicts in
core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cppcore/shader/recompiler/PipelineSpecialization.hppdocs/dev/debt/shader.md
Please rebase it onto current main. I will review the change once it merges cleanly.
3a6f213 to
ed711db
Compare
702dee4 to
48e75dd
Compare
|
@XairRrR thanks for the approval. #9183 has since landed and the branch conflicted, so it is rebased onto
Apart from that assert, With the validation layer on the RTX 3060 Laptop: the 31 image gather, sample and table tests pass 30/31, including |
XairRrR
left a comment
There was a problem hiding this comment.
Re-checked — approved
The branch was rebased onto main 086c0686c8 after #9183 landed. Both 2D and 3D gather branches stay in EmitGatherOp, and the first static_assert now starts from VolumeGatherBase. Checks pass, the negative control fails, and the PR adds no new validation IDs.
What changed since my last review
- Rebase onto
main086c0686c8, which now has #9183 (VolumeGatherBase57664,EmitVolumeGatherLz).- In commits 2–5, the intermediate ids are now 57856, 57888 and 57920.
- The final ids are unchanged:
GatherLodBase57856,GatherLodBiasBase57920,GatherImageMinLodBase57984.
DescriptorBindingBuilder.cpp:22: the first assert is nowVolumeGatherBase + MaxImages <= GatherLodBase. This is 57664 + 64 = 57728 ≤ 57856.SpirvImageEmitter.cpp:1695-1702: #9183's 3D branch returns before the 2D explicit-LOD branch at lines 1739-1753, and the two don't interact.EmitTwoDimensionalGatherLodis identical to what I reviewed before.- Its guard still requires
Dim2D.
- The description and the 20:10 reply describe this rebase. Both say that apart from the assert the diff is the one I approved.
delta.patchagrees: commits 4 and 5 are=, and the other changes are context lines and the id lines above.
What I ran
Head 48e75dd9d096d738f9318d49329ad050081e4b09, main 086c0686c8adff3ee648fdbe34e0b0b647e1d511, merged with --no-ff into tree be2973f389, in a clean worktree. Debian 13 VM, GCC 14.2.0, lavapipe (Mesa 26.1.6). This device has no VK_EXT_image_view_min_lod.
git diff --check 086c0686c8...48e75dd9d0
python3 tools/check_conventions.py --base 086c0686c8
VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn,layer \
ctest --test-dir build -V -R '^agc_driver_image_gather_(lod|volume)$'
# negative control: restore the six production files from 086c0686c8
ninja -C build -j32 -l32 agc_driver_image_gather_lod_tests agc_driver_image_gather_volume_tests
VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 \
ctest --test-dir build -V -R '^agc_driver_image_gather_(lod|volume)$'
# restore from be2973f389, rebuild, rerun
ninja -C build -j32 -l32 agc_driver_image_gather_lod_tests agc_driver_image_gather_volume_tests
ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build --output-on-failure -R '^agc_driver_image_gather_(lod|volume)$'
git status --porcelain| Check | Result |
|---|---|
git diff --check |
Clean |
check_conventions.py |
All conventions checks passed |
| Build (script) | OK |
| Selected tests (script) | 2188 run, no new failures, no timeouts. The 7 failures are all known on main |
| Script's validation diff | 784 tests; the layer loaded in 783 and was missing in none. One test has "new" VUID-s: agc_driver_image_gather_volume, which was not in the older baseline (see below) |
agc_driver_image_gather_lod, with the validation layer |
Passed (0.95 s). The layer was inserted, there were no VUID- lines, and the T# MIN_LOD cases were skipped (no extension) |
agc_driver_image_gather_volume, with the validation layer |
Passed (9.6 s). It logs four VUID-VkSamplerCreateInfo-addressMode* IDs |
Same two tests with main's production files |
gather_lod fails. gather_volume passes and logs the same four VUID-s |
| After restoring the merged files | Both pass, and git status --porcelain is empty |
Negative control output (main's production files)
1234: AGC graphics: clamp to edge, point: thread 1 (LOD 1.000000, level 1) component 0 is 108, expected 113
1/2 Test #1234: agc_driver_image_gather_lod ......***Failed 0.19 sec
1237: Validation Error: [ VUID-VkSamplerCreateInfo-addressModeU-parameter ]
1237: Validation Error: [ VUID-VkSamplerCreateInfo-addressModeV-parameter ]
1237: Validation Error: [ VUID-VkSamplerCreateInfo-addressModeW-parameter ]
1237: Validation Error: [ VUID-VkSamplerCreateInfo-addressModeU-01079 ]
2/2 Test #1237: agc_driver_image_gather_volume ... Passed 9.61 sec
The script's validation baseline is main 1e658521a0, which predates #9183, so it marked agc_driver_image_gather_volume as "test not run on main". With this PR's production files reverted to 086c0686c8, the test logs the same four IDs. They come from main's mirror-once sampler created without samplerMirrorClampToEdge, as the author says, and do not count against this PR.
Previous findings
- Specialization ids collide for sampler indices 32–63 (blocking, resolved in my last review): still resolved. The bases are 57856, 57920 and 57984, each sampler range is 64 wide, and the image range is now chained from
VolumeGatherBase(57664–57727). The four asserts compile on the merged tree.
New findings
Blocking
None.
Non-blocking
- The debt entry is out of date for 3D images after the rebase.
docs/dev/debt/shader.md:252lists "3D and cube images" among those that "still gather the base level". Onmainsince #9183, though, a 3D gather without theLZflag fails compilation with "has an unsupported 3D gather variant" (SpirvImageEmitter.cpp:1594-1597onmain;SpirvImageEmitter.cpp:1695-1698in the merged tree). So a 3Dimage_gather4_lthrows; it does not gather the base level. Suggested fix: drop "3D" from the base-level list, and end the entry with "on 1D and 3D imagesimage_gather4_lthrows".
Not verified
- The T#
MIN_LODvariants were skipped, because lavapipe has noVK_EXT_image_view_min_lod. Results on devices that have it are author- or contributor-reported, and the contributor runs were on earlier heads. - No shader with more than 32 samplers that gathers at an explicit LOD was compiled or run.
- I did not check the id use of the open PRs named in the description (#10489, #14621); only
mainis in this tree. - The PPSA23276 in-game result and the cost numbers are author-reported and predate this rebase.
|
Windows and NVIDIA Blackwell run, for head
The failure, Not run: the Khronos validation layer is not installed on this machine, so this says nothing about VUIDs. PPSA23276 was not run. AI-assisted: yes. Claude Code ran the builds and tests and drafted this comment; the numbers are from the runs on this machine. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (4, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (8 IDs in 527 tests)Skipped (50)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
✅ AMD GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (5, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validated but not compared (57, not in the main baseline's validation pass)Validation IDs already present on main (8 IDs in 518 tests)Skipped (57)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Independent RDNA 3 / Windows check of head On the selected AMD Radeon RX 7800 XT ( This exercises the four-component gather/readback comparison against a CPU 2×2 footprint across 32 LOD inputs and the test's clamp, wrap, mirror, fractional LOD, sampler LOD clamp/bias, mip-filter-none, UINT-with-bilinear-sampler, and border-fallback cases. It adds a current-head RX 7800 XT Windows result alongside the existing RDNA 4 Windows, RADV, and NVIDIA reports. I did not run PPSA23276 or a paired The installed validation layer uses Vulkan header version 284 and emitted |
SPIR-V gathers take no LOD without SPV_AMD_texture_gather_bias_lod, so image_gather4_l gathered the base level. Sample the four footprint texels at their centres with the explicit LOD instead. UE5's Nanite HZB occlusion test reads mip 0 otherwise and culls geometry behind narrow gaps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Specialize samplers of image_gather4_l on their LOD state (MIN_LOD, MAX_LOD, LOD_BIAS, MIP_FILTER none, CLAMP_X/Y), select the level in the shader with float clamps before any conversion, and fetch the footprint texels at that level instead of sampling them, so the sampler's clamp and filter cannot move or blend them. Border clamp modes and sampler tables keep the base-level gather. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…m LOD Specialize the images of explicit-LOD gathers on their T# MIN_LOD above the base level and raise the chosen LOD to it after the sampler's clamp, so the fetched level never falls below the view's integer minimum LOD. Clamp the sampler LOD in an explicit order (MIN_LOD, then MAX_LOD) and keep images and samplers read from a table on the base-level gather. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… view's minimum level With MIP_FILTER none the fetch took level 0, below the view's integer minimum LOD when the T# MIN_LOD is at least one level above the base level. It now takes the T# MIN_LOD above the base level rounded down, clamped to the view's last level. The graphics-pipeline-library prebuild no longer builds shaders with an explicit-LOD gather sampler: it specializes on zeroed sampler words, so a draw never uses that module. The mirror-once test variant is dropped until the sampler creates MIRROR_CLAMP_TO_EDGE with samplerMirrorClampToEdge enabled; the debt entry now says 1D image_gather4_l throws and 2D arrays gather the base level. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…view_min_lod Without the extension the texture path refuses a descriptor with a minimum LOD clamp before the gather runs, so the two T# MIN_LOD variants failed the whole test on such devices (lavapipe, MoltenVK). They now run only when the device has it; the other variants run everywhere. Reported, with this change, by jotitadev in the PR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
main raised ShaderInfo::MaxSamplers to 64 (12b8923), but the GatherLodBase and GatherLodBiasBase ranges stayed 32 wide, so sampler 32's LOD state shared an id with sampler 0's bias, and sampler 32's bias with image 0's GatherImageMinLodBase value. Each sampler range is now 64 wide, and DescriptorBindingBuilder asserts that each range fits before the next base, from VolumeGatherBase up to ImageComponentBase. The ranges start at 57856 (GatherLodBase 57856, GatherLodBiasBase 57920, GatherImageMinLodBase 57984, with room for 128 images up to 58111), above VolumeGatherBase (57664) and the ids open pull requests take (FmaskFormatBase at 57728-57855 with 128 images, SamplerLodBiasBase at 57664), and below DeviceSamplerClassBase at 58368. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
48e75dd to
51d4bda
Compare
✅ AMD GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (6, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validated but not compared (55, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 551 tests)Skipped (68)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (9 IDs in 592 tests)Skipped (56)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (9 IDs in 593 tests)Skipped (55)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |


What
image_gather4_lon a 2D image gathered the base level:EmitGatherOpemittedOpImageGatherwithout the instruction's LOD, because SPIR-V gathers take no Lod operand withoutSPV_AMD_texture_gather_bias_lod. Such a gather now picks its level in the shader and fetches the four footprint texels there (EmitTwoDimensionalGatherLod,OpImageFetch). The level follows the sampler: samplers used byimage_gather4_lare specialized on their S# LOD state (MIN_LOD,MAX_LOD,LOD_BIAS,MIP_FILTERnone,CLAMP_X/CLAMP_Y;PipelineSpecialization::GatherLodBase/GatherLodBiasBase, new useSamplerUseGatherLod); λ = lod + bias, raised toMIN_LODand lowered toMAX_LOD, raised to the T#MIN_LODabove the base level (the view's own minimum LOD; images specialized throughGatherImageMinLodBase), clamped to the view's levels in floating point, nearest level with ties down as in Vulkan (ceil(λ + 0.5) - 1). ForMIP_FILTERnone the level is the T#MIN_LODabove the base level rounded down, clamped to the view's levels, so the fetch never reads below the view's minimum LOD. The wrap modes are applied to the footprint asGuestSamplerResourcemaps them. Fetches do no filtering, so integer and unfilterable formats are fine with any guest filter. Images or samplers from a table and the border modes (4, 6) keep the base-level gather.Semantics: RDNA2 ISA, MIMG opcodes (
IMAGE_GATHER4_L: gather 4 single-component elements (2x2) with a user LOD); the 2x2 footprint and its order follow the Vulkan specification, Texel Gathering: i0 = floor(u * width - 0.5), j0 = floor(v * height - 0.5), results (i0, j1), (i1, j1), (i1, j0), (i0, j0); level selection follows Vulkan's LOD operation for a nearest mip filter. Not measured on hardware; the unmeasured parts are indocs/dev/debt/shader.md.Effect on PPSA23276: UE5's Nanite instance and cluster culling tests occlusion with four
image_gather4_lat the HZB mip that covers an object's screen rectangle and takes the minimum. Reading 2x2 texels of mip 0 instead made the test non-conservative: wall panels behind narrow gaps were culled in both the main and the post pass, so the recess in the right wall and part of the end of the first hallway rendered black, as reported in #8587.New execution test
ImageGatherLod: a 16x16 RGBA8 image with five mips,image_gather4_lwith 32 LODs per variant (integers, 0.49/0.5/0.51/1.49/1.5/2.5, negatives, 4294967296.0, +inf, NaN) in twelve variants (clamp to edge, wrap, mirror, bilinear x/y, MIN_LOD 1.25/MAX_LOD 2.75, MIN_LOD = MAX_LOD = 2, LOD_BIAS -0.75, MIP_FILTER none, 8_8_8_8_UINT with a bilinear sampler, T# MIN_LOD 2.5, MIP_FILTER none with T# MIN_LOD 2.5, border modes 6 and 4 on the base-level gather), against the footprint computed on the CPU. The two T# MIN_LOD variants run only on devices withVK_EXT_image_view_min_lod, because without it the texture path refuses such a descriptor, or on the shader-clamp lane a gather at that clamp. Mirror-once coverage is left for thesamplerMirrorClampToEdgefix:GuestSamplerResourcecreatesMIRROR_CLAMP_TO_EDGEwithout that feature, which adds validation errors of its own.Tested
Windows 11 Pro 10.0.26200, NVIDIA GeForce RTX 3060 Laptop GPU, driver 592.82, GCC 15.2.0 (WinLibs UCRT).
Rebased onto
main086c0686c8.mainnow gives 57536 toMirrorUnnormalizedBase, 57600 toMinLodBaseand, since #9183, 57664 toVolumeGatherBase(64 ids each), so the gather constants moved above them.mainalso raisedShaderInfo::MaxSamplersto 64 (12b8923), so the sampler ranges are 64 wide:GatherLodBase57856 andGatherLodBiasBase57920 (64 samplers each),GatherImageMinLodBase57984 (room for 128 images, up to 58111). They also start above the ids open PRs take: #10489 raisesMaxImagesto 128 and movesFmaskFormatBaseto 57728-57855, #14621 addsSamplerLodBiasBaseat 57664;DeviceSamplerClassBase(#7642 and others) starts at 58368.static_asserts inDescriptorBindingBuilder.cpptie each range toShaderInfo::MaxSamplersorMaxImages, fromVolumeGatherBaseup toImageComponentBase. Otherwise the diff is the one reviewed before;SpirvImageEmitter.cppkeeps #9183's 3D gather and this PR's 2D explicit-LOD gather side by side.GatherImageMinLodBasecarries the same T# clamp asMinLodBasefrom fe18496, but in 1/256 steps and on every device, whereMinLodBaseis a float written only on the shader-clamp lane, so I kept them apart. That lane still refuses a gather at a clamp of a level or more, so the two T# MIN_LOD variants stay skipped withoutVK_EXT_image_view_min_lod.ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, the 31 image gather, sample and table tests (ctest -R '^(agc_driver_image_(gather|sample|texel_status|table|packed)|image_sample_gather)'): 30/31 on this branch at086c0686c8, all twelveImageGatherLodvariants and fix(shader): gather a 3D image's image_gather4_lz from one slice as measured #9183'sagc_driver_image_gather_volumepass, and both lanes ofagc_driver_image_sample_lod_clamppass. The one failure isagc_driver_image_tableon itsSYNC-HAZARD, which fails onmaintoo. Validation IDs:VUID-VkDeviceCreateInfo-pNext-pNext, andVUID-VkSamplerCreateInfo-addressMode*fromagc_driver_image_gather_volumealone, whose mirror-once sampler is created withoutsamplerMirrorClampToEdge(main's code; this PR creates no samplers).agc_driver_image_gather_lodwithAPS5_NO_IMAGE_VIEW_MIN_LOD=1 APS5_NO_GPL=1(the shader-clamp lane): passes, the two T# MIN_LOD variants skipped.VK_EXT_image_view_min_lodwas run here by forcing the check off; this GPU has the extension. On lavapipe, the same change passed 3/3 in jotitadev's run.python3 tools/check_conventions.py --base origin/main: passed.main9090972b7e(the same diff except the three ids and the asserts) againstmainat that commit, both built the same way:main, 251 compute pipelines on both, cache 563 MiB against 562 MiB, recompile time 3.2-3.4 s on both. The culling samplers keep one LOD state in this scene, so the specialization adds no pipeline here.APS5_PROFILE_GPU=1, last 110 s: 26.1 and 26.6 ms per present against 26.0 and 27.0 onmain, 60.5 presents per 10 s against 58.8-60.2.main. One cluster cull went from 226-237 us to 452-457 us, the other from 272-280 us to 226-228 us; the instance cull (108 us) and the indirect culls are unchanged. I have not separated the extra fetches from the different culling work: with the right mip the culls keep instancesmaindrops, so the later passes get different inputs.main+ fix(shader): load and store 16-bit data of converted packed images #11580 + a local depth-plane fix that goes in its own PR, 300 s): the panels at the far right of the first hallway were black in every shot without this change (60-75 % of the gap region near-black) and render in every shot with it (0 %); near-black share of the whole frame 1.6-1.7 % -> 0.4-0.5 %; frame rate unchanged (55-57 presents per 10 s).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