[SPIR-V] Add SPV_EXT_descriptor_heap + SPV_KHR_untyped_pointers codegen - #8517
[SPIR-V] Add SPV_EXT_descriptor_heap + SPV_KHR_untyped_pointers codegen#8517Jonathan Zakharov (jzakharovnv) wants to merge 9 commits into
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
f640327 to
cbcae38
Compare
|
@microsoft-github-policy-service agree company="NVIDIA" |
Diego Novillo (dnovillo)
left a comment
There was a problem hiding this comment.
Thanks for this! I just started looking at it and have a couple of questions. I'll add more as I read the PRs.
| SpirvEmitter::getDescriptorHeapRuntimeArrayType(const SpirvType *elemType, | ||
| bool onSamplerHeap) { | ||
| constexpr uint32_t kDefaultResourceHeapStride = 64; | ||
| constexpr uint32_t kDefaultSamplerHeapStride = 32; |
There was a problem hiding this comment.
I think I would float these defaults to SpirvEmitter.h and document where the seemingly magic values 32 and 64 come from.
There was a problem hiding this comment.
Definitely agree that these need some better documentation. I placed them at this scope because they seem a little niche, and I want to try to avoid polluting very wide scopes, but I can move them up.
There was a problem hiding this comment.
With the inclusion of OpConstantSizeOfEXT, per Tobski, these default stride constants have been removed.
| constexpr uint32_t kDefaultSamplerHeapStride = 32; | ||
| const uint32_t stride = | ||
| onSamplerHeap ? kDefaultSamplerHeapStride : kDefaultResourceHeapStride; | ||
| return spvContext.getRuntimeArrayType(elemType, stride); |
There was a problem hiding this comment.
It doesn't seem that there are tests for OpDecorate ArrayStride N in this PR. I haven't checked the others. Could you add one (unless I missed it?)
There was a problem hiding this comment.
Yes, there is a healthy amount of testing in the last 2 PRs of this series, but I neglected to have for this one. I can fix that.
| addExtension(Extension::EXT_descriptor_heap, "DescriptorHeap", {}); | ||
| addExtension(Extension::KHR_untyped_pointers, "DescriptorHeap", {}); | ||
| const llvm::StringRef feature = "DescriptorHeap"; | ||
| featureManager.requestTargetEnv(SPV_ENV_VULKAN_1_3, feature, {}); |
There was a problem hiding this comment.
This function can return failure (which is getting dropped here), and there is no test verifying its error handling.
There was a problem hiding this comment.
Thanks for catching that, I'll add a test.
There was a problem hiding this comment.
| tryToAssignToDescriptorHeapBuffer(expr)) | ||
| return aliasResult.getValue(); | ||
|
|
||
| auto *rhs = loadIfGLValue(expr->getRHS()); |
There was a problem hiding this comment.
LLVM's coding standards which DXC adopts (although not historically well enforced), have an "almost never auto" policy:
The heavy use of auto in this code makes it significantly harder to review this PR in a browser without an IDE telling me the types of things.
There was a problem hiding this comment.
Noted, thanks for tip!
There was a problem hiding this comment.
There was a problem hiding this comment.
The re-base removed the auto suggestion from Chris B (@llvm-beanz) Could you re-apply it?
| if (result && !result->isRValue()) { | ||
| result = | ||
| spvBuilder.createLoad(structType, result, buffer->getExprLoc(), range); | ||
| } |
There was a problem hiding this comment.
| if (result && !result->isRValue()) { | |
| result = | |
| spvBuilder.createLoad(structType, result, buffer->getExprLoc(), range); | |
| } | |
| if (result && !result->isRValue()) | |
| result = | |
| spvBuilder.createLoad(structType, result, buffer->getExprLoc(), range); |
There was a problem hiding this comment.
Will fix in next commit
There was a problem hiding this comment.
Handled with Remove unnecessary braces in descriptor heap code
| } | ||
|
|
||
| // ConstantBuffer -> Uniform (UBO); all others -> StorageBuffer (SSBO) | ||
| // TODO: Remove this manual override once LowerTypeVisitor returns the |
There was a problem hiding this comment.
Should this be fixed before we merge this change? Seems like you're working around a clear bug here.
There was a problem hiding this comment.
To get the storage class here rather than in LowerTypeVisitor is bit of a bandage but provides functionally correct output. I had left this TODO in with the aim of making this PR as small/unintrusive as possible and with room to improve with future commits. If you're okay with filing an issue, let me know, otherwise I can go about cleaning up this fix.
There was a problem hiding this comment.
I think I'm okay with using an issue to track that as a follow up. Normally it would be better to do that foundational work first to avoid committing code in an undesirable state unless there's a reason why it can't be done before this change merged.
There was a problem hiding this comment.
Understandable. I am open to clean the code up, it just may be a while before I am able to address everything (on the order of 2 weeks). Let me know if that is what you prefer!
There was a problem hiding this comment.
Circling back! It has been a busy few weeks, but I am open to fixing this now if it means it could be reviewed before SIGGRAPH (July 19th). Would this be possible for you, or can we file an issue instead?
There was a problem hiding this comment.
There was a problem hiding this comment.
|
|
||
| float4 main(uint idx : A) : SV_Target { | ||
| Texture2D<float4> tex = ResourceDescriptorHeap[NonUniformResourceIndex(idx)]; | ||
| SamplerState samp = SamplerDescriptorHeap[NonUniformResourceIndex(idx + 1)]; |
There was a problem hiding this comment.
This seems like something we should have the compiler issue a diagnostic on. Silently dropping something the user explicitly wrote seems unfortunate.
There was a problem hiding this comment.
I agree, this is a bit esoteric but correct according to Tobski. How should we handle a diagnostic here, just a simple warning?
There was a problem hiding this comment.
Oh... Actually I think the correct solution for HLSL is that in the absence of NonUniformResourceIndex an access should be marked as Uniform, and NonUniformResourceIndex suppresses that.
It should have an effect, so no diagnostic should be required.
|
Jonathan Zakharov (@jzakharovnv) I think the OpConstantSizeOfEXT usage in this MR isn't quite right - it looks like you're just taking the stride of the descriptor type being loaded and using that? This works for the sampler heap where there's only one type (samplers), but not for the resource heap. The stride of all resource descriptors must be the same for all descriptors, and has to be based on the biggest of the buffer/image sizes. This is illustrated in the VK extension doc here: https://docs.vulkan.org/features/latest/features/proposals/VK_EXT_descriptor_heap.html#_shader_model_6_6_samplerheap_and_resourceheap I might be misunderstanding the code, but I don't see anything for finding the largest of the two sizes? They're both POT, so it is a simple "which is max" calculation, but I don't see it being done in this PR. |
|
Tobski You are right, this is my blunder. Will fix shortly. |
|
Tobski Shared max(image,buffer) stride for descriptor heap resource arrays ought to be closer to the original intent. Please take a look when you can. Thanks! |
Looks right to me now! Thanks! |
Diego Novillo (dnovillo)
left a comment
There was a problem hiding this comment.
A question on changes that have gone in #8519 that we may to reflect here. Not sure how you want to handle it.
| } | ||
|
|
||
| // ConstantBuffer -> Uniform (UBO); all others -> StorageBuffer (SSBO) | ||
| // TODO: Remove this manual override once LowerTypeVisitor returns the |
There was a problem hiding this comment.
bd79339 to
cbcae38
Compare
|
Diego Novillo (@dnovillo) In response to your last comment on this PR, yes I think it's more correct to have the TODO commit a part of this initial branch rather than one down the line. Cherrypicked and rebased to reflect this. |
Diego Novillo (dnovillo)
left a comment
There was a problem hiding this comment.
Just one minor change and it's good to from my side.
| tryToAssignToDescriptorHeapBuffer(expr)) | ||
| return aliasResult.getValue(); | ||
|
|
||
| auto *rhs = loadIfGLValue(expr->getRHS()); |
There was a problem hiding this comment.
The re-base removed the auto suggestion from Chris B (@llvm-beanz) Could you re-apply it?
Diego Novillo (dnovillo)
left a comment
There was a problem hiding this comment.
LGTM. Just one final nit. Thanks for doing this!
| if (result && !result->isRValue()) { | ||
| result = | ||
| spvBuilder.createLoad(structType, result, buffer->getExprLoc(), range); | ||
| } |
There was a problem hiding this comment.
Could you re-apply the suggestion from Chris B (@llvm-beanz)? It got dropped.
Building off of microsoft#8281, this commit adds a native lowering via SPV_EXT_descriptor_heap and SPV_KHR_untyped_pointers. ResourceDescriptorHeap and SamplerDescriptorHeap are lowered to untyped variables decorated with ResourceHeapEXT and SamplerHeapEXT. Each heap access emits OpUntypedAccessChainKHR into a runtime array of the appropriate descriptor type. Buffer-like resources (StructuredBuffer, ByteAddressBuffer, ConstantBuffer, TextureBuffer) use OpTypeBufferEXT and OpBufferPointerEXT; image and sampler resources use OpLoad. Interlocked operations on RWTexture use OpUntypedImageTexelPointerEXT. Requires -fspv-target-env=vulkan1.3. Assisted-by: Claude.
d0835c8 to
88bd33c
Compare
|
There was a problem hiding this comment.
Pull request overview
Adds native SPIR-V descriptor-heap lowering using SPV_EXT_descriptor_heap and SPV_KHR_untyped_pointers.
Changes:
- Implements native image, sampler, and buffer descriptor access.
- Adds descriptor-size-based runtime-array strides and image atomic support.
- Expands tests and documents Vulkan 1.3 requirements and limitations.
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tools/clang/unittests/SPIRV/SpirvContextTest.cpp |
Tests runtime-array type uniqueness. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.typed-formats.hlsl |
Tests typed image formats. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.texturecube.hlsl |
Tests cube textures and samplers. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.texture.hlsl |
Tests texel buffers. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.texture-sampler-assignment.hlsl |
Tests resource reassignment. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.texture-ms.hlsl |
Tests multisampled textures. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.texture-dims.hlsl |
Tests sampled texture dimensions. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.structured-buffer-atomic.hlsl |
Tests structured-buffer atomics. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.static-global.hlsl |
Tests static global resources. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.sampler-comparison.hlsl |
Tests comparison samplers. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.sample-grad-bias.hlsl |
Tests sampling operands. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.rwtexture-dims.hlsl |
Tests RW texture dimensions. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.rwtexture-atomics.hlsl |
Tests untyped image atomics. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.rwbyteaddressbuffer.hlsl |
Tests writable byte buffers. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.nonuniform.hlsl |
Tests divergent heap indexing. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.mixed-bound.hlsl |
Tests bound/native resource coexistence. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.mixed-alias.error.hlsl |
Tests mixed-alias diagnostics. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.load-offset.hlsl |
Tests texture load offsets. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.groupshared.hlsl |
Tests groupshared coexistence. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.gather.hlsl |
Tests gather operations. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.function-params.hlsl |
Tests resource function parameters. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.discarded.error.hlsl |
Tests discarded-index diagnostics. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.counter-ops.error.hlsl |
Tests unsupported counter diagnostics. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.constant-texture-buffer.hlsl |
Tests constant and texture buffers. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.constant-buffer-assignment.hlsl |
Tests constant-buffer reassignment. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.buffer.hlsl |
Tests native buffer lowering. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.array-stride.hlsl |
Tests descriptor-array strides. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.append-consume.error.hlsl |
Tests append/consume diagnostics. |
tools/clang/test/CodeGenSPIRV/resource-heap-ext-texture.hlsl |
Removes superseded coverage. |
tools/clang/lib/SPIRV/SpirvType.cpp |
Extends runtime-array equality. |
tools/clang/lib/SPIRV/SpirvInstruction.cpp |
Implements new SPIR-V instructions. |
tools/clang/lib/SPIRV/SpirvEmitter.h |
Declares heap lowering and alias state. |
tools/clang/lib/SPIRV/SpirvEmitter.cpp |
Implements native heap code generation. |
tools/clang/lib/SPIRV/SpirvContext.cpp |
Uniques buffer and stride-ID types. |
tools/clang/lib/SPIRV/SpirvBuilder.cpp |
Builds descriptor sizes and strides. |
tools/clang/lib/SPIRV/LowerTypeVisitor.cpp |
Lowers untyped image pointers. |
tools/clang/lib/SPIRV/EmitVisitor.h |
Declares new emission handlers. |
tools/clang/lib/SPIRV/EmitVisitor.cpp |
Serializes new instructions and decorations. |
tools/clang/lib/SPIRV/DeclResultIdMapper.h |
Declares function alias registration. |
tools/clang/lib/SPIRV/DeclResultIdMapper.cpp |
Implements alias registration. |
tools/clang/lib/SPIRV/CapabilityVisitor.cpp |
Requires Vulkan 1.3 and extensions. |
tools/clang/include/clang/SPIRV/SpirvVisitor.h |
Adds visitor hooks. |
tools/clang/include/clang/SPIRV/SpirvType.h |
Adds stride-ID runtime arrays. |
tools/clang/include/clang/SPIRV/SpirvInstruction.h |
Defines new instruction classes. |
tools/clang/include/clang/SPIRV/SpirvContext.h |
Adds type caches and APIs. |
tools/clang/include/clang/SPIRV/SpirvBuilder.h |
Exposes descriptor-stride builders. |
docs/SPIR-V.rst |
Documents native descriptor heaps. |
| const bool srcIsHeap = isDescriptorHeap(srcExpr->IgnoreParenCasts()); | ||
| const bool wasHeap = descriptorHeapImageAliasVars.count(dstVar) || | ||
| descriptorHeapBufferAliasVars.count(dstVar); | ||
| const bool wasBound = stateIt != descriptorHeapVarState.end() && | ||
| stateIt->second == DescriptorHeapVarState::Bound; | ||
| const bool mixingDetected = | ||
| (srcIsHeap && wasBound) || (!srcIsHeap && wasHeap); |
| } else if (const auto *varDecl = dyn_cast<VarDecl>(decl)) { | ||
| if (auto *alias = | ||
| emitDescriptorHeapBufferPointer(varDecl, expr->getLocStart())) | ||
| result = alias; |
| const SpirvType *handleType = | ||
| lowerTypeVisitor.lowerType(resourceType, SpirvLayoutRule::Void, | ||
| llvm::None, baseExpr->getExprLoc()); | ||
| const auto *arrayType = | ||
| spvContext.getRuntimeArrayType(handleType, llvm::None); | ||
| auto *untypedAccessChainPtr = spvBuilder.createUntypedAccessChainKHR( | ||
| untypedType, arrayType, var, index, baseExpr->getExprLoc()); | ||
| const SpirvType *arrayType = | ||
| getDescriptorHeapRuntimeArrayType(handleType); |
| A local resource variable initialized from a heap access is resolved entirely at | ||
| compile time: the variable is recorded as an alias for the heap index, and every | ||
| later use is re-lowered as a fresh access chain rather than as a load of a stored | ||
| descriptor handle. This is sound only when the variable holds a heap descriptor |
| // CHECK-DAG: %[[SampSize:[a-zA-Z0-9_]+]] = OpConstantSizeOfEXT %uint %[[SamplerDesc]] | ||
|
|
||
| // Every resource runtime array shares the one resource stride. | ||
| // The sampler array just uses the sampler size as it's stride. |
Fixes #8603: MergeBinaryOpSelect produced an OpSelect with a vector result type and a scalar condition, which is only legal in SPIR-V 1.4+ (KhronosGroup/SPIRV-Tools#6827). This exposes #8740 where the resource-heap-ext-texture.hlsl test fails validation - for now this is worked around by disabling the test, since #8517 looks like it is reworking it anyway. Assisted-by: copilot --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b04b21af-2d8b-4af5-b9ad-1b84073588f5
|
Jonathan Zakharov (@jzakharovnv) Damyan Pepper (@damyanp) Gregory Roth (@pow2clk), I have gone over all the open review threads for all the PRs in this series #8517, #8518 and #8519. I would like to get them to the point where we can submit them so that other developers can start testing this implementation. I went over the reviews and tried to separate blocking issues from issues we could postpone in a follow-up PR or even open an issue to fix later. I would like to get consensus on what we need done to get some forward progress. Given that this feature is protected by a flag, I don't think is should be too destabilizing to add it even when there are still things to fix. I don't want to have a broken initial version, but I also don't want to spend a long time getting it to be perfect before it goes in. We need the feature exposed to developers so they can start testing it. I'll add all my notes here and will put cross-references in the other PRs so I don't lose track. These are the things I think should be fixed now before it goes in. These were signaled by Copilot on 4/Aug: Two blocking issues for #8517
One blocking issue for #8518
Two blocking issues for #8519
Everything else I am happy to defer, and I do not think it should hold back the series:
|
Five issues triaged in isolated parallel sessions against a Debug build of ab54009, collated by a separate session briefed only by on-disk artifacts. | # | verdict | history | | --- | --- | --- | | 2530 | repros | always (20/20 releases) | | 3055 | repros | v1.4.1907 output byte-identical to main | | 3259 | repros | always from v1.5.2010 | | 8725 | repros | always from v1.8.2505 (5 of 20 can express lib_6_9) | | 8732 | inconclusive | unmeasurable -- filed against unmerged PR microsoft#8517 | microsoft#8732 is the useful outlier: every symbol it blames has zero occurrences on main, so it is not does-not-repro -- the symptom was never in the compiler being measured. It needs a human decision, not a fix. The headline tooling fix is the invalid-probe classifier, which batch 004 predicted would be least trustworthy where the reported symptom is itself a diagnostic. It was, in both directions: a release emitting a correct, complete diagnostic -- what a fix looks like on such an issue -- scored invalid-probe and could hide the fixing release; and a probe whose predicate had *matched* was still discarded because the symptom text is itself a marker, erasing ground truth and then misattributing it to the profile. microsoft#3055 escaped only because dxc says "no matching member function for call to" where the marker is "no matching function for call to". The fix is deliberately narrow: a demotion is suppressed only when the issue's own match.json positively quotes the marker text. Making the classifier broadly more permissive would reintroduce the fake-regression bug it exists to prevent, which has already produced wrong verdicts twice. Verified against every archived predicate -- no legitimate demotion is lost, and a reindex moved exactly the two probes that demonstrated the defect out of 346. Also fixed: reindex now re-scores variant captures, which previously were never checked against the current classifier -- its first run found three stale headers in microsoft#2202 that had been wrong since batch 004; invalid-probe verdicts now record why on disk; `is not supported` is anchored to a target/profile clause; ce_args() valueless-flag handling; a warning when --args diverges from cmd.txt; a labels --refresh REST fallback. ~30 new tests. Ground truth was rebuilt for this batch (upstream/main merged, SPIRV-Tools submodule to 1c336172, cached version headers cleared) because microsoft#8732 sits in the blast radius of ec2ba18. That proved load-bearing rather than precautionary: the reporter's own workaround compiles on v1.9.2607 but fails on main under a newly enforced UniformConstant ArrayStride rule. The stale build would have reported the feature's state incorrectly. Batch 005's verdicts are therefore measured against a different compiler than batches 001-004; triaged_with_commit records this per issue. Report-only: no issue was edited, commented on or closed, and no DXC source is touched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6bd9cb60-aeb2-4d6c-9462-7a5e79fe1cda
Building off of #8281, this PR adds a native lowering via SPV_EXT_descriptor_heap and SPV_KHR_untyped_pointers and is part 1/4 in a series.
ResourceDescriptorHeap and SamplerDescriptorHeap are lowered to untyped variables decorated with ResourceHeapEXT and SamplerHeapEXT. Each heap access emits OpUntypedAccessChainKHR into a runtime array of the appropriate descriptor type. Buffer-like resources (StructuredBuffer, ByteAddressBuffer, ConstantBuffer, TextureBuffer) use OpTypeBufferEXT and OpBufferPointerEXT; image and sampler resources use OpLoad. Interlocked operations on RWTexture use OpUntypedImageTexelPointerEXT.
Requires -fspv-target-env=vulkan1.3.
Assisted by an AI agent.
Diego Novillo (@dnovillo)