Repository navigation
feat(shader): support 32-bit image atomics through keyed image tables - #8792
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
I found a remaining StorageView in the recorded image-table descriptor update path. In ShaderResources::buildComplete(), the binding.flags != 0 branch around line 2349 constructs VkDescriptorImageInfo with storageTextures[index]->StorageView(...) unconditionally. The compact-binding path below it (around line 2394) now selects AtomicView when storageAtomic[index] is set, but table descriptors use the recorded branch. Since this PR marks table elements atomic in resolveImageTables(), the table can still be written with the non-atomic view and the new execution tests may fail or trigger Vulkan validation there. Please make the recorded table write use the same storageAtomic64 / storageAtomic view selection as the compact path, then run the image-table tests with validation when available. Hosted GPU/build jobs are skipped on the current PR head, so they do not verify this branch.
|
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.
Correction to my earlier review: I misread the recorded table descriptor branch. It already selects AtomicView for atomic entries (and Atomic64View for 64-bit entries in the compact branch); the table image-info construction shown here is correct. I withdraw my request for changes. The PR checks currently pass for eligibility, conventions, intake, labels, and report; build/runtime jobs are skipped, so I have not independently verified the GPU execution claim.
|
Thanks for the careful review and the correction, MarcZX10 — glad the recorded table branch reads correctly on re-read. GPU execution evidence is in the PR body (RTX 2060 SUPER runs); happy to add any extra run you want. Leaving it in the maintainers' queue. |
|
Windows data point for
|
|
An AMD proprietary-driver run: RX 6700 XT (gfx1031), AMD Windows driver 32.0.21045.5002, Windows 11 26200, MinGW GCC 15.2.0, Release,
On the float-record case: integer |
|
Thanks xXPBSXx and thuanquang — the reverted-to-main control failing 3/3 and the AMD + validation 0-VUID runs are exactly the evidence this PR needed, including the float-record confirmation against your HIP oracle rows. Much appreciated. |
|
@SuarezPM thanks for the update. The full CI run on a55b76e (merged into current main) fails three tests on Linux, and the cause is in this PR:
Why it shows up only there: only the Linux CI lane configures with Cause: before this PR, 32-bit image atomics never reached the keyed-table path, so the table merge in // SpirvImageEmitter.cpp:1971
if (returnsValue) incoming.insert(incoming.end(), {ConstantU32CompositeZero(state, 4u), invalid});The sampler-guard Phi at :1988 has the same constant. |
|
Fixed as suggested: both merge sites now use |
a55b76e to
40f112e
Compare
Table T#s keyed at runtime no longer fault on image_atomic_*32: the table operation stays supported, elements carry the atomic flag through dedup into the descriptor binding, and the table path binds AtomicView like the direct path. 64-bit atomics stay unsupported, packed and BY formats stay unsupported, and the debt entry records the relaxed Device-scoped emission and the remaining gaps.
Table accessors returning scalar uint fed the merge Phi a 4-wide zero, failing SPIR-V validation with SPIRV_TOOLS on. Use ZeroResult like the unsupported path.
40f112e to
0ef31ba
Compare
|
Re-checked after main moved: the SRT branch-condition change doesn't interact with the table atomic path, and the code merges cleanly. Only the |
What
32-bit image atomics through keyed image tables no longer fault. A T# keyed at runtime feeding
image_atomic_*32now lowers toOpAtomic*on the table'sAtomicViewinstead ofunsupported operation, so compute using bindless UAVs stops dying on the first atomic dispatch. Follows up the stores support from #6402, which left atomics faulting ("Atomic, packed, BY2/BY4 and dynamic-mip table stores still fault").ResourceTracker::AssignTableskeepstableOperationsupported for 32-bit atomics; 64-bit atomics, packed and BY formats still markUnsupported.DescriptorBindingBuilder,ShaderResources), and the table write path bindsAtomicViewexactly like the direct path.docs/dev/debt/shader.mdrewrites the image-table entry: 32-bit atomics supported (Devicescope, relaxed,AcquireRelease/UniformMemorybarrier as the direct path); faulting remainder isimage_store_mip/image_store_pck, PCK2/PCK4, MIMG BY2/BY4 and 64-bit atomics.Tested
Host: Linux x86-64 (CachyOS), Ryzen 5 3600, RTX 2060 SUPER (NVIDIA 615.78.08). No validation layer installed on this host, so no VUIDs could be produced here; full builds run on merge.
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)