Repository navigation
test(agc): expect no kept image table writes for a set from the shared pool chain - #10651
maximilianfv wants to merge 1 commit into
Conversation
…d pool chain The image table reuse checks expect the table's descriptor set to come from a dedicated pool, which DescriptorCache keeps and whose writes it counts. A set gets a dedicated pool only when one of its sizes exceeds the shared pool chain; with MoltenVK's limits the sampler table holds 192 entries and the set fits the chain, so no table write is recorded and the test failed with 0 kept and 0 made. Detect from the descriptor cache counters that the set came from the chain and then require that no table write was recorded. Co-authored-by: Ycaro Gabriel <280482597+ycarogabriel-lgtm@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 3beed54422b3bc90e84670e4dc08d291f63ca343. The test classifies the descriptor set from the first two dispatches: dedicated allocations and reuse of a kept dedicated set increment the counters, while shared-chain allocations do not. The production code also records table writes only for dedicated allocations with a non-null layout, so requiring zero writes for the shared-chain case is consistent with the implementation. The test preserves the original kept-write assertions when those counters show a dedicated allocation. Exact-head diff-check, conventions, and merge-tree checks pass; hosted eligibility, conventions, intake, labels, and report pass, while product builds are skipped. I did not run the GPU test locally; I reviewed the author’s M1 Pro run and control results. No blocking issue found.
|
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.
A new overlap check for #6223 prompted a manual three-way merge comparison. Both PRs edit ImageTableReuse.cpp; the current two-head merge-tree reports a shared-hunk conflict around the write-count helper and the RunTests dispatch section. The code additions appear conceptually compatible—#6223 adds per-element reuse assertions while this PR handles shared-pool write counters—but the branches need an agreed integration order/rebase so neither assertion set is dropped. The PR body currently says they change neither the write helper nor the first dispatches, while the overlap notice shows the same region and the checklist says Depends on: none. Please coordinate with #6223’s author and record the landing order (or explain how the current heads merge without losing either test) before merge. The prior exact-head functional review remains valid; this request is only for the newly surfaced coordination requirement.
|
Landing order with #6223, checked against
I didn't build or run the combined tree. This was a patch-level check only. |
|
Windows check of the dedicated-pool path on three non-Apple GPUs.
"Both" is |
What
agc_driver_image_table_reuseandagc_driver_image_table_reuse_draw_pipelineassume that the image table's descriptor set comes from a dedicated pool. That assumption does not hold on every device. The kept-table write counters (writesKept,writesMade), added with kept descriptor sets in #6454 by EagleBearerYS (commit 1d66d85), count writes only for a set thatDescriptorCache::Allocatetakes from a dedicated pool.ShaderResources.cpprecords table writes only whensampled && set < tableWrites.size() && allocation.layout != VK_NULL_HANDLE, andallocation.layoutis set only in the dedicated-pool branch. A set gets a dedicated pool only when one of its sizes exceedsChainPoolSizes(4096 storage buffers, 1024 sampled images, 1024 storage images, 512 samplers). A set from the shared pool chain is freed back to the chain, so the driver has nothing to keep and correctly records nothing.The only size that can exceed the chain is the sampler table.
SamplerTableCapacityForsizes it asmin(1024, Room(stage samplers, 64), Room(set samplers / 4, 64)), first from the plain limits and, when that gives 0, from the update-after-bind limits. MoltenVK on the M1 Pro reports (vulkaninfo)maxPerStageDescriptorSamplers16,maxPerStageDescriptorUpdateAfterBindSamplers1024 andmaxDescriptorSetUpdateAfterBindSamplers1024. The plain limits give 0 (16 is below the 64-sampler heap), so the update-after-bind limits apply: min(1024, 960, 192), rounded down to a multiple of 4, is 192, below the chain's 512 samplers. The test's own counters confirm it:dedicatedPools + reusedSetsdoes not grow across the first two dispatches, which is why both tests print the chain line in the runs below. The test then fails at its first write check:three dispatches of one table in one epoch: 0 table descriptor writes kept and 0 made, expected 3 and 0.This change reads
descriptorCache->Counters()before and after the first dispatches. IfdedicatedPools + reusedSetsdid not grow, the table's set came from the shared pool chain. The test then printsthe table's descriptor set fits the shared pool chain: kept table sets are not tested, and everyExpectWritesrequires 0 kept and 0 made table writes. On devices whose sampler table reaches 1024, the set still comes from a dedicated pool and the original expectations run unchanged. WithAPS5_NO_POOL_CACHEthe descriptor cache is bypassed, so the test also takes the 0/0 path; the expectation is still correct, only the printed reason is then imprecise. The driver is not changed.Prior art: of the whole-port PRs, #7201 and #8658 (and the PRs #8658 was split into, such as #10419) do not touch this test, but the closed #7332 (
feat: macOS on Apple Silicon host support, by Ycaro Gabriel) has the same diagnosis and a test fallback in commits fba033d and 4fab753 (2026-10-19). This PR reimplements that fallback with a different check: #7332 madeExpectWritestake the device and require 0 kept and 0 made writes wheneverCounters().dedicatedPoolswas 0 for the device as a whole, reaching the context through a newVulkanDeviceTestAccessstruct; this PR compares the counters around this test's own first dispatches and leaves theExpectWritessignature as it is. Ycaro Gabriel is credited as co-author. Testers on #8658 reportedagc_driver_image_table_reusefailing on an M1 with the same message and passing on an M4, so this expectation depends on the GPU's limits. Open #6340 (MarcZX10) and #6223 (Thunderhawkk) also edit this test, but they add element-reuse checks and change neitherExpectWritesnor the first dispatches.Tested
Apple M1 Pro, macOS 27.0.1, x86_64 build under Rosetta 2, Vulkan through MoltenVK 1.4.2 from the LunarG Vulkan SDK 1.4.363.
mainalone does not build on macOS. I tested in a tree made of mugurc's macOS series (ci/macos-series-2, 27e9689) merged withmainat 522e32e, plus these local changes, all identical in every run: the macOS build parts of #10419, #10540, #10562, #10641 and #10642, and two of my unopened fixes (the posix_spawnp child in LazyResolvedAbi.cpp, an OpMemoryBarrier before 32-bit image atomics in SpirvImageEmitter.cpp). Only this PR's change was toggled. All runs usedANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationandctest --test-dir build -R '^agc_driver_image_table_reuse' -E 'audio_in|voice' -V.agc_driver_image_table_reuseandagc_driver_image_table_reuse_draw_pipelinefail (0 of 2) withthree dispatches of one table in one epoch: 0 table descriptor writes kept and 0 made, expected 3 and 0.the table's descriptor set fits the shared pool chain: kept table sets are not tested.APS5_KEPT_DESCRIPTOR_SETS=0, both pass (2 of 2) and each prints the same chain line.Validation Errorlines, bothVUID-vkCreateDescriptorSetLayout-support-09582, and nowas not found but was requested by env var VK_INSTANCE_LAYERSline.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