Repository navigation
fix(agc): find a template's data bindings by binding number - #9564
Conversation
A resource template keeps no binding of its own for an image table: the build hands the shader's image table binding to the table stage. Its binding list is therefore one shorter than the shader's, yet DataWordsDiffer and refreshData paired the two lists by index and required equal lengths. Every data refresh of a compute template whose shader reads an image table threw "template bindings disagree with the shader", and DataWordsDiffer reported different words for the shader the template was built from. Evidence: a title stopped once its pipelines came from the cache with "compute shader 0x80003c7100: AGC graphics: template bindings disagree with the shader". The shader listed 9 bindings (guest buffers, three guest image arrays, the fault buffer, the flattened SRT, the sampler table, the image table map and the image table at 260); the reused template held 8, without binding 260. Both functions now look up the template binding of each data binding by its binding number. Tested: agc_driver_graphics builds a compute template from an image table map, an empty image table and shader data, checks that its own words compare equal, refreshes it with other words, and checks the data buffer and the compare afterwards. Without the change it fails with "a template's own words differ from the shader that built it". Cherry-picked from fable-void's 7bb699b; conflict in tests/Graphics.cpp resolved to main's image table ABI: the test's header no longer sets HeaderImageSlots, a word main's ABI does not have (main keeps HeaderElementBase at that index), and its mock context gets the image table limits main's imageTableAllocationTests sets (image table capacity and sampled images per set and per stage, 2 each), since main refuses image tables on a context without them.
With APS5_PROFILE_DRAW set, the [rescache] line now counts the template hits that took a data refresh while the template held fewer bindings than the shader (no image table binding of its own): the refreshes that threw "template bindings disagree with the shader" before the bindings were paired by binding number.
|
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.
|
|
Early review feedback while this remains a draft: the focused test exercises the image-table binding-count mismatch and verifies both the template’s own data words and refreshed dispatch words. Before marking it ready, please coordinate the shared |
There was a problem hiding this comment.
The binding-by-number fix addresses the real image-table template shape: the table binding is consumed by the table stage, so data bindings can no longer be paired safely by vector index. DataWordsDiffer and refreshData now locate each data binding by layout.binding; refresh still validates the single held allocation, size, and shader-data runtime constraints. The regression creates a shader with an image-table map, an empty image table, and shader data, then checks the template's own words, changed words, the refreshed buffer, and the post-refresh comparison. I found no correctness issue in the reviewed change. The hosted Linux/Windows red/green runs include 10 repeats of the focused graphics test and changed executables; the PR also reports the required same-runner Khronos validation comparison with the layer confirmed loaded and no new VUIDs. Hosted eligibility, intake, labels, report and conventions pass; product Linux/Windows and relinker jobs are skipped. I could not run another local build because the workspace filesystem is full, so these are author-reported CI results, not local runs. The branch is 2 ahead / 116 behind current main (94fbb8cea merge base), cleanly mergeable; please refresh before merge. Current main conventions and git diff --check pass. I checked #9508's current branch: it adds DrawGuestWords for descriptor word access, and the authors agree either merge order is fine. This PR's binding-number lookup should be retained together with that source-of-words change when #9508 is integrated; it is not currently in this branch's ancestry. The automated overlap report is partial because many old PR comparisons are uncomparable against main, and it lists the shared files; no competing implementation of this specific image-table refresh fix was found. Approval is for this reviewed head only.
I’ve noticed that holding this PR because of #5239 isn’t necessary. It’s fine for PRs to keep coming in; they’ll be handled in due course. Please disregard my earlier request to hold or defer it.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The binding-number lookup is the right fix for the shorter template binding list, and the reports exercise both DataWordsDiffer and refreshData. The Windows/Linux red/green runs, repeated focused tests, and paired validation-layer run are strong evidence; hosted checks show conventions/eligibility/intake/labels/report pass while product and relinker jobs are skipped. I did not run a local GPU build.
I refreshed current upstream/main to 329efe1bfdaff116c57cdf62718026ee4f5521ae. This head is 2 ahead / 161 behind (merge base 94fbb8cea80af91db62ab6ee7e4da23341675c40). Current-base merge-tree and conventions pass, and git diff --check passes, but main has since changed ShaderResources.cpp and related AGC paths, including HTILE and image-format handling. Please rebase onto current main and rerun the focused template-refresh test plus the relevant paired Vulkan validation selection on the refreshed tree before merge.
#9508 remains open at 9f5e875b95d6a5bbee580b03b36ed7773fd80aa0; pairwise merge-tree currently conflicts in Graphics/src/ShaderResources.cpp. Your description records the author agreement that either can land first and preserves DrawGuestWords with binding-number lookup. Keep that agreed no-wait resolution and both behaviors when integrating; there is no duplicate implementation of this precise binding-index fix.
|
Thanks, @MarcZX10. |
|
I’ve noticed that my earlier request to hold this PR based on #5239 was unnecessary. It’s fine to keep submitting PRs; maintainers will handle them in due course. Please disregard that request. |
Dismissing a stale review: the requested changes were addressed after it (new head and/or updated description with the asked-for runs) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Checked the lookup by |
What
A resource template keeps no binding of its own for an image table: the build hands the shader's image table binding to the table stage. So its binding list is one shorter than the shader's. Yet
DataWordsDifferandrefreshDatapaired the two lists by index and required equal lengths. Every data refresh of a compute template whose shader reads an image table threw "template bindings disagree with the shader".DataWordsDifferalso reported different words for the very shader the template was built from.This is fable-void's 7bb699b from his fork (
wip/new-gpu-path), with his authorship:main's image table ABI: its header no longer setsHeaderImageSlots, whichmaindoesn't have, and its mock context gets the image table limitsmain'simageTableAllocationTestssets.APS5_PROFILE_DRAWset, the[rescache]line also counts the template hits refreshed while the template held fewer bindings than the shader,N paired their data bindings by number (no image table binding of their own). Without the profile it costs one test of astatic const bool.fable-void's evidence, from his commit: a title stopped, once its pipelines came from the cache, with "compute shader 0x80003c7100: AGC graphics: template bindings disagree with the shader". The shader listed 9 bindings (guest buffers, three guest image arrays, the fault buffer, the flattened SRT, the sampler table, the image table map, and the image table at 260). The reused template held 8, without binding 260. No open issue carries that text.
Overlap: checked with
git merge-treeagainst every open PR touching these files, onmaindf2569d. One conflicts:DataWordsDifferandrefreshData: it reads the words throughDrawGuestWords(shader, binding)and keeps the pairing by index. The two compose by looking each binding up by number and reading its words throughDrawGuestWords. Neither waits for the other, agreed in perf(agc): GPU descriptor heap on VK_EXT_descriptor_heap (APS5_GPU_HEAP) #9508's thread (reply): if perf(agc): GPU descriptor heap on VK_EXT_descriptor_heap (APS5_GPU_HEAP) #9508 merges first, this PR rebases, and if this one does, the resolved merge goes on a fork branch for perf(agc): GPU descriptor heap on VK_EXT_descriptor_heap (APS5_GPU_HEAP) #9508 to take (note).Tested
agc_driver_graphicsbuilds a compute template from an image table map, an empty image table and shader data, on its mock Vulkan device. It checks that the template's own words compare equal, refreshes it with other words, then checks the data buffer and the compare.main's code, it fails ×3 on both runners, with "a template's own words differ from the shader that built it".agc_driver_graphics_maps_fallback, the same executable under the maps fallback, fails with it on Linux.precise_wait, a timing test that flakes on that runner.maincf2c667: 1812/1818, with the same 6 failures asmain.mainagainst this PR (CONTRIBUTING's AGC rule), on one GitHub Linux runner (run). Both runs usedVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1,ANYPS5_REQUIRE_VULKAN=1andctest -V, withVK_LOADER_DEBUG=layerto show the layer loading. The device was llvmpipe (LLVM 20.1.2) on Mesa 25.2.8, Ubuntu 24.04.5, Khronos validation layers 1.3.275.agc_driver_graphicson the mock device and creates no Vulkan instance, so the comparison adds real-device tests that reuse a cached draw or dispatch with new data, theRefreshDataandDataWordsHashpath this PR changes:agc_driver_draw_cache_reuse,agc_driver_draw_cache_reuse_verify,agc_driver_draw_data_hit_user_words,agc_driver_draw_data_hit_user_words_verify,agc_driver_image_table_reuseandagc_driver_buffer_load_store. With the tests whose build inputs this PR touches, that's 35 on each side;main's tree ran them once and this PR's three times.VK_LAYER_KHRONOS_validation10 times onmainand 30 with this PR, and neither log haswas not found but was requested by env var VK_INSTANCE_LAYERS.VUID-VkDeviceCreateInfo-pNext-pNext, for a device-creation structure newer than that layer version. This PR adds noVUID-message.mainb7d1d47 merged in (clean; the PR's own diff is unchanged), on GitHub's runners (run):agc_driver_graphicsand the other tests pass 10 of 10 on Linux, none skipped. On Windows all 34 build and pass, with 6 skipping without a Vulkan device.agc_driver_graphicsfails withmain's implementation. On Linux this run was in validation mode, whosemainarm buildsmain's whole tree, tests included, so it runsmain'sagc_driver_graphicswithout this PR's new case. The Linux red is from the first run.mainagainst this head, on the Linux runner: the loader insertedVK_LAYER_KHRONOS_validation10 times onmainand 100 with this PR, and neither log haswas not found. Both logs report onlyVUID-VkDeviceCreateInfo-pNext-pNext, so this PR adds noVUID-message.APS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.check_conventions.pypasses, with no comment lines added.APS5_PROFILE_DRAW=1; no fatal error,[rescache]reports0 paired their data bindings by number (no image table binding of their own), so this title doesn't reach the case either.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)agc_driver_graphics's mock Vulkan device, on GitHub's Linux (Ubuntu 24.04) and Windows runners; the validation comparison ran on Mesa llvmpipe (LLVM 20.1.2, Mesa 25.2.8), Ubuntu 24.04.5, GitHub's Linux runner.