Fix #11506: accept heap texel pointer as atomic destination#11507
Draft
nv-slang-bot[bot] wants to merge 2 commits into
Draft
Fix #11506: accept heap texel pointer as atomic destination#11507nv-slang-bot[bot] wants to merge 2 commits into
nv-slang-bot[bot] wants to merge 2 commits into
Conversation
Under -capability spvDescriptorHeapEXT, the descriptor-heap lowering
rewrites an image-subscript atomic destination into an
IRSPIRVLoadTexelPointerFromHeap. The deferred atomic-destination
validator isValidAtomicDest, which runs after that rewrite, accepted
IRImageSubscript but had no case for the heap texel pointer, so it
rejected the atomic with error E41403 ("invalid atomic destination").
A plain RWTexture2D keeps its IRImageSubscript and always validated.
Accept IRSPIRVLoadTexelPointerFromHeap as a valid atomic destination,
mirroring the existing IRImageSubscript case; the op already emits as
the atomicable OpUntypedImageTexelPointerEXT.
Adds a compile-only regression test. The emitted SPIR-V still fails
binary validation because bindless image handles carry image format
Unknown (tracked separately by #11130, with and without the
capability), so the test skips binary validation and FileChecks the
emitted opcodes.
Co-authored-by: Harsh Aggarwal <haaggarwal@nvidia.com>
Address advisory clarity nits from PR #11507 review: broaden the function's contract comment to cover image/texel destinations (it omitted IRImageSubscript even before this change), and expand the heap-texel-pointer comment with the deferred-validation order and sole-producer facts that make unconditional acceptance safe. Comment-only; no behavior change. Co-authored-by: Harsh Aggarwal <haaggarwal@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
InterlockedAdd(any atomic) on a texel of aDescriptorHandle<RWTexture2D<uint>>failed to compile witherror[E41403]: invalid atomic destination— but only under-capability spvDescriptorHeapEXT. A plainRWTexture2D<uint>(or the sameDescriptorHandlewithout the capability) did not hit E41403. This is a false-negative in the IR atomic-destination validator; the fix teaches it about the descriptor-heap-lowered texel pointer.Diagnosis (root cause)
spvDescriptorHeapEXT,processImageSubscript(source/slang/slang-ir-spirv-legalize.cpp:1357-1370) replaces theIRImageSubscriptatomic destination with anIRSPIRVLoadTexelPointerFromHeap(a pointer inAddressSpace::Image). The plain-texture branch (:1352-1355) leaves theIRImageSubscriptin place.source/slang/slang-emit.cpp:1917-1921) and runs at the end of legalization (source/slang/slang-ir-spirv-legalize.cpp:2789) — i.e. after that rewrite.isValidAtomicDest(source/slang/slang-ir-validate.cpp:455-527) acceptsIRImageSubscript(:463) and an address-space allowlist (:468-487), but has no case forIRSPIRVLoadTexelPointerFromHeapand noAddressSpace::Imagecase → falls through toreturn false(:526) →InvalidAtomicDestinationPointer/ E41403.OpUntypedImageTexelPointerEXT(source/slang/slang-emit-spirv.cpp:4934-4959), atomicable just likeOpImageTexelPointer. So emission was already correct — only the IR-level validator was out of date.Approach
Accept
IRSPIRVLoadTexelPointerFromHeapinisValidAtomicDest, immediately after the existingIRImageSubscriptcase — the post-lowering l-value form of the same construct the validator already accepts pre-lowering. One added check.Alternatives considered and rejected:
case AddressSpace::Image: return true;— would bless any Image-space pointer as an atomic destination; less safe than recognizing the exact heap-texel-pointer opcode. (Independent review concurred: prefer the opcode check.)Files changed
source/slang/slang-ir-validate.cpp— acceptIRSPIRVLoadTexelPointerFromHeapas a valid atomic destination (+ a short rationale comment).tests/bugs/gh-11506-descriptor-handle-texture-atomic.slang— new compile-only regression test.Tests
error[E41403]: invalid atomic destination.OpUntypedImageTexelPointerEXT+OpAtomicIAdd.slang-test.tests/spirv/atomic*,tests/spirv/descriptor-heap*,tests/spirv/image-atomic*,tests/bugs/vk-image-atomics, etc.), 8 GPU-only tests ignored.clang-formatis not available in this environment, soextras/formatting.shwas not run locally; the change matches the surrounding style and CI's format check covers it.This PR fixes only the spurious E41403 atomic-validator error. The front-end / IR path now compiles and emits the correct ops, but end-to-end binary SPIR-V still fails
spirv-valfor bindless texture atomics, because the bindless image type carries image formatUnknown:This missing-format defect is orthogonal to the atomic validator and is tracked separately by #11130. It reproduces for
DescriptorHandle<RWTexture2D<uint>>atomics both with and withoutspvDescriptorHeapEXT(a plainRWTexture2D<uint>infersR32uiand validates; bindless handles getUnknown). It lives on the bindless image-format surface (see also #11499, #11503), not the atomic validator. Accordingly, the regression test uses-skip-spirv-validationand FileChecks the emitted opcodes (format-agnostic, so it stays green once #11130 lands).Risk
Validation-only; the change widens what counts as a valid atomic destination by exactly one opcode that is produced solely on the texture-atomic heap-lowering path, so it cannot newly reject any existing program. No public header / ABI / language-surface change.
Fixes #11506.