[SPIR-V] Add descriptor heap RaytracingAccelerationStructure support - #8518
[SPIR-V] Add descriptor heap RaytracingAccelerationStructure support#8518Jonathan Zakharov (jzakharovnv) wants to merge 28 commits into
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
d899b05 to
b12b9ea
Compare
|
@microsoft-github-policy-service agree company="NVIDIA" |
b12b9ea to
23eca97
Compare
23aff82 to
02f3581
Compare
Diego Novillo (dnovillo)
left a comment
There was a problem hiding this comment.
Only a question left on detecting RT usage of the heap.
6293723 to
20dd181
Compare
There was a problem hiding this comment.
Pull request overview
Extends native SPIR-V descriptor-heap lowering to acceleration structures and broader resource operations.
Changes:
- Adds descriptor-size-based heap strides, including acceleration structures.
- Adds native buffer, image, sampler, atomic, and alias lowering.
- Expands SPIR-V tests and documentation.
Reviewed changes
Copilot reviewed 51 out of 51 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
tools/clang/unittests/SPIRV/SpirvContextTest.cpp |
Extends runtime-array uniqueness tests. |
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 gradient and bias sampling. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.rwtexture-dims.hlsl |
Tests storage-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 nonuniform-index handling. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.mixed-bound.hlsl |
Tests bound and heap resources together. |
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 interoperability. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.gather.hlsl |
Tests texture 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-access 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 shared heap strides. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.array-stride.accelstruct.hlsl |
Tests acceleration-structure stride inclusion. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.append-consume.error.hlsl |
Tests append/consume diagnostics. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.acceleration-structure.stride.error.hlsl |
Tests missing acceleration stride diagnostics. |
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.acceleration-structure.hlsl |
Tests acceleration-structure heap loads. |
tools/clang/test/CodeGenSPIRV/resource-heap-ext-texture.hlsl |
Removes the superseded aggregate test. |
tools/clang/lib/SPIRV/SpirvType.cpp |
Includes stride IDs in type 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 lowering and acceleration support. |
tools/clang/lib/SPIRV/SpirvContext.cpp |
Canonicalizes buffer and runtime-array types. |
tools/clang/lib/SPIRV/SpirvBuilder.cpp |
Builds descriptor-size constants 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 |
Emits new instructions and decorations. |
tools/clang/lib/SPIRV/DeclResultIdMapper.h |
Declares function-variable aliases. |
tools/clang/lib/SPIRV/DeclResultIdMapper.cpp |
Registers function-variable aliases. |
tools/clang/lib/SPIRV/CapabilityVisitor.cpp |
Enforces Vulkan 1.3 for native heaps. |
tools/clang/lib/SPIRV/AstTypeProbe.cpp |
Recognizes acceleration-structure types. |
tools/clang/include/clang/SPIRV/SpirvVisitor.h |
Adds visitor methods. |
tools/clang/include/clang/SPIRV/SpirvType.h |
Stores runtime-array stride IDs. |
tools/clang/include/clang/SPIRV/SpirvInstruction.h |
Defines new instruction classes. |
tools/clang/include/clang/SPIRV/SpirvContext.h |
Extends type-context APIs. |
tools/clang/include/clang/SPIRV/SpirvBuilder.h |
Exposes descriptor-stride builders. |
tools/clang/include/clang/SPIRV/AstTypeProbe.h |
Declares acceleration-type probing. |
docs/SPIR-V.rst |
Documents native descriptor heaps. |
Comments suppressed due to low confidence (1)
tools/clang/lib/SPIRV/SpirvEmitter.cpp:7072
- Only casts are stripped here, so a syntactically valid parenthesized heap base such as
(ResourceDescriptorHeap)[i]is diagnosed as unsupported even though it still refers directly to the builtin variable. UseIgnoreParenCasts()consistently when resolving the base declaration.
const auto *declRefExpr = dyn_cast<DeclRefExpr>(baseExpr->IgnoreCasts());
const auto *decl =
declRefExpr ? dyn_cast<VarDecl>(declRefExpr->getDecl()) : nullptr;
if (!decl) {
emitError("unsupported ResourceDescriptorHeap/SamplerDescriptorHeap "
"expression",
baseExpr->getExprLoc());
return nullptr;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
20dd181 to
568efbb
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
tools/clang/lib/SPIRV/SpirvEmitter.cpp:5315
- A heap-to-heap reassignment of
RaytracingAccelerationStructurealso reaches this branch becausemixingDetectedincludes(isASType && wasHeap)regardless ofsrcIsHeap. That assignment does not mix bound and heap resources, so this diagnostic is misleading; report the unsupported AS reassignment separately.
emitError("mixing bound and descriptor heap resources in the same variable "
"is not supported with SPV_EXT_descriptor_heap",
loc);
tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.array-stride.hlsl:62
- Use the possessive “its” rather than “it's” (“it is”).
// The sampler array just uses the sampler size as it's stride.
docs/SPIR-V.rst:2089
- This is a significant user-visible compiler feature, but
docs/ReleaseNotes.mdhas no native descriptor-heap entry. Please add release-note coverage, or point to the shared release note planned for this four-PR series.
When ``-fspv-use-descriptor-heap`` is specified, DXC lowers
``ResourceDescriptorHeap`` and ``SamplerDescriptorHeap`` through
``SPV_EXT_descriptor_heap`` instead of the default emulated heap path. This
also requires ``SPV_KHR_untyped_pointers`` and ``-fspv-target-env=vulkan1.3``
|
Added an analysis on this series: #8517 (comment) |
abba0d5 to
0446c2c
Compare
0446c2c to
381ec18
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Direct heap counter operations bypass the intended diagnostic, and several user-facing messages and release-note details need correction.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
docs/ReleaseNotes.md:31
- Repository release-note rules state that links to specific PRs should not be included (
CONTRIBUTING.md:136-139). Remove this PR link from the entry.
[#8518](https://github.com/microsoft/DirectXShaderCompiler/pull/8518).
- Files reviewed: 59/59 changed files
- Comments generated: 4
- Review effort level: Balanced
| (void)doExpr(object); | ||
| } | ||
|
|
||
| if (isDescriptorHeapCounterUnsupported(object)) { |
| if (mixingDetected) { | ||
| emitError("mixing bound and descriptor heap resources in the same variable " | ||
| "is not supported with SPV_EXT_descriptor_heap", | ||
| loc); |
Extends the SPV_EXT_descriptor_heap native heap lowering to cover RaytracingAccelerationStructure resources loaded from ResourceDescriptorHeap. Acceleration structure descriptors are accessed via OpUntypedAccessChainKHR into a runtime array of OpTypeAccelerationStructureKHR, consistent with the image and sampler paths added in the previous commit.
381ec18 to
7f59f1c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Direct buffer-like heap expressions bypass function-boundary and counter-operation safeguards, potentially producing invalid lowering or misleading fatal diagnostics.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
tools/clang/lib/SPIRV/SpirvEmitter.cpp:3531
- This guard only catches a local alias represented by a
DeclRefExpr; a direct argument such asconsume((StructuredBuffer<uint>)ResourceDescriptorHeap[0])bypasses it.doExprthen produces the native buffer-data pointer, and the ordinary parameter lowering can attempt to materialize/store the whole buffer value, which is precisely the unsupported cross-function representation this diagnostic is meant to prevent. Check for any heap-sourced buffer-like argument before theDeclRefExprspecialization.
if (const auto *declRefExpr = dyn_cast<DeclRefExpr>(arg)) {
// Buffer alias vars are not in astDecls (getDeclEvalInfo would crash);
// passing by value also requires VariablePointersStorageBuffer + callee
// parameter type changes (not yet implemented). Emit diagnostic instead.
// TODO: implement full buffer-alias function-call support
// (VariablePointersStorageBuffer + matching createFnParam type).
const auto *var = dyn_cast<VarDecl>(declRefExpr->getDecl());
if (var && descriptorHeapBufferAliasVars.count(var)) {
tools/clang/lib/SPIRV/SpirvEmitter.cpp:5260
- Direct heap expressions are not present in
descriptorHeapBufferAliasVars, so((RWStructuredBuffer<uint>)ResourceDescriptorHeap[0]).IncrementCounter()is not recognized here. It falls through to counter lookup and emits the unrelated fatal “array of buffers in a struct” error instead of the documented descriptor-heap counter diagnostic.isHeapSourcedValue(expr)already recognizes both direct accesses and aliases, so use it for this predicate.
bool SpirvEmitter::isDescriptorHeapCounterUnsupported(const Expr *expr) const {
if (const auto *var = dyn_cast_or_null<VarDecl>(getReferencedDef(expr))) {
auto it = descriptorHeapBufferAliasVars.find(var);
if (it != descriptorHeapBufferAliasVars.end())
return it->second.counterUnsupported;
}
return false;
- Files reviewed: 59/59 changed files
- Comments generated: 1
- Review effort level: Balanced
| auto *decl = cast<VarDecl>(declRefExpr->getDecl()); | ||
| declIdMapper.createResourceHeap(decl, resourceType); | ||
| if (isHeapSourcedValue(srcExpr)) { | ||
| if (isDescriptorHeap(srcExpr)) { |
Building off of #8517, this PR extends the SPV_EXT_descriptor_heap native heap lowering to cover RaytracingAccelerationStructure resources loaded from ResourceDescriptorHeap. It is part 2/4 in a series.
Acceleration structure descriptors are accessed via OpUntypedAccessChainKHR into a runtime array of OpTypeAccelerationStructureKHR, consistent with the image and sampler paths added in the previous commit.
Assisted by an AI agent
Diego Novillo (@dnovillo)