Repository navigation
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the exact head 5106dae. The change removes dispatch-time buffer-presence specialization from the cache identity and derives it from the descriptor words at runtime. The specialized GCN path masks the high-base word before testing its low 16 bits, and the formatted-buffer helper now receives validity from its existing runtime parameter. The GuardedMemoryLoads fixture encodes descriptor validity in the V# words, and the shader disk-cache format is bumped. I found no correctness issue in those paths.\n\nLocal checks run: and All conventions checks passed. both pass. I did not run the local GPU test suite because no usable build tree is available and the root filesystem is full. I inspected the reported same-machine NVIDIA baseline/change results: the author reports 1,140/1,143 selected tests passing on both, the same three existing sample-order failures and identical pre-existing VUID sets, plus matching warm capture hashes across eight large programs. Hosted relinker/product build jobs are policy-skipped for this PR, so I am relying on the author-provided runtime report for GPU validation.
|
Correction to the review text: the local commands were |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the four-file change and the reported same-machine main/change Vulkan validation runs. The dispatch-time path now derives buffer presence from V# words at run time; the formatted-buffer path uses the existing runtime parameter, and the disk-cache version is bumped for the SPIR-V change. The fixture now encodes validity in descriptor words rather than specialization, matching the production behavior. The author reports the same three known test failures and VUID set on main and the change, with no new validation errors; required intake/conventions checks pass. Approved at head 5106dae.
|
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.
|
Hardware & Environment
Test Methodology & Verification
LGTM! AI-assisted: yes. Written with AI assistance and reviewed by me. |
|
Prior art I missed when opening this: #5353 (@Pipodi) moved both buffer stride and presence to run time for the same Yōtei compute programs. It was closed on Oct 17 because, with cold caches, the first frame came later (261 s) and total compile work didn't drop. How this one differs:
The format-word variant Pipodi found on Oct 22 (discussion #675) is a separate key; this PR doesn't touch it. This post was prepared with AI assistance (Claude); I reviewed the results. |
|
Intel run, since the generated SPIR-V changes and the runs above are NVIDIA only. Head 5106dae was merged into Setup: Windows 11 Pro 10.0.26300, WinLibs GCC 15.2.0 r7, Release. Environment:
AI-assisted: yes (Claude Code) |
|
@mrfelek this now conflicts with main in |
5106dae to
2e0c5c3
Compare
|
@oneandonlydean Rebased onto main 887a964 and took FormatVersion 47; full ctest with validation matches main (same 3 sample_order failures). AI-assisted. Tested by me on my own machine. |
|
@mrfelek Thanks for the rebase. A correction to my earlier ask, sorry for the churn: this PR doesn't need a FormatVersion bump at all. It changes runtime SPIR-V generation, not the encoded cache layout (there's no ShaderDiskCache.cpp change), and each changed runtime source already changes SourceVersion, which keys the cache directory and is checked on read. Could you drop the ShaderDiskCache.hpp FormatVersion edit and keep main's value? Several other open PRs need the next numbers. |
…alizing it A buffer V#'s presence came from the dispatch's V# snapshot as the specialization constant BufferBase + 2, which buildSpecializedModule turns into a literal. One guest program then compiled to two SPIR-V modules, one with the buffer bound and one with it still null, each a separate pipeline. descriptor() now reads presence from the V# words at run time, as registration modules already do, and the formatted-buffer helper takes it from its valid parameter. AI-assisted. Tested by me on my own machine.
2e0c5c3 to
2031190
Compare
|
@oneandonlydean done: dropped the FormatVersion change and kept main's value; the PR now only touches the runtime SPIR-V and its test. AI-assisted. Tested by me on my own machine. |
…me) into local/merge23 Our tree already read presence at run time (runtimePresent) and took the formatted helper's valid from its parameter; kept ours and added the PR's GCN word1 mask to runtimePresent. The BufferBase + 2 specialization is no longer recorded. FormatVersion 52 -> 53. AI-assisted: yes
…ykopovar#10228, Linux build fix) AI-assisted: yes
What
A buffer V#'s presence (
dwords[2] != 0 && (dwords[0] != 0 || (dwords[1] & 0xffff) != 0)) was supplied as specialization constantBufferBase + i*BufferWords + 2from the dispatch's V# snapshot, andbuildSpecializedModuleturns supplied specializations into literal constants. The same guest program therefore compiled to two SPIR-V modules, one where the buffer was bound and one where it was still null, with differentRecompileResult::Identity()and no VkPipelineCache hit between them; which one a dispatch got depended on timing.PPSA26344 has eight compute programs of 1.2–1.45M SPIR-V words reached in both states; every new state cost a 54–320 s NVIDIA pipeline compile, and pipeline journal replay of the other state did not help.
descriptor()in SpirvRuntimeBuffer.cpp now computes presence from the V#'s words at run time, as the registration-module path already does; the formatted-buffer helper takes it from its existingvalidparameter, andDescriptorBindingBuilderno longer suppliesBufferBase + 2. The GuardedMemoryLoads fixture now encodes validity in the V# words instead of the specialization. ShaderDiskCache FormatVersion is bumped because generated SPIR-V changes.Tested
Ubuntu 26.10, Ryzen 9 5950X, RTX 3080 Ti 12 GB, NVIDIA 615.71.09. PPSA26344 02.002.000 title-scene load (with a separate blend change applied locally to get past its earlier stop).
BufferBase+2: 1 vs 0) and 20OpLogicalAndfolding it. Warm run: pipelines over 100k words took 1,221 s in the first 15 min because 5 journal-replayed giants were rebuilt at 54–70 s each.tools/select_tests.py, 1143 selected): 1140 pass; the same 3 failures (agc_driver_sample_order,agc_driver_sample_order_grid_reference,agc_driver_sample_order_recipe_depth_locations) occur on main 895f47f on this machine.ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationon main and on this change: the same 3 failures and identical VUID sets (four pre-existing kinds), no new validation errors.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)Related: #639 added registration-time modules, which already read presence from the V# at run time; this brings the dispatch-time specialization path in line.
Conflicts with #7484 only in the GuardedMemoryLoads test fixture (
emitLoad/AccessTracesignatures); whichever lands second needs a small fixture rebase.AI-assisted. Tested by me on my own machine.