Use uint64 AS descriptor heap stride#11494
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughType RaytracingAccelerationStructure descriptor-heap entries as uint64 and emit an 8-byte ArrayStride for that heap case; update SPIR-V emission, tests, and documentation to reflect the stride exception while preserving prior stride derivation for other resource types. ChangesRAS descriptor heap stride and uint64 element type
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad8ec485-f247-4c35-8a9e-6c0f130a5b3c
📒 Files selected for processing (7)
docs/command-line-slangc-reference.mddocs/user-guide/03-convenience-features.mddocs/user-guide/08-compiling.mdsource/slang/slang-emit-spirv.cppsource/slang/slang-options.cpptests/spirv/descriptor-heap-acceleration-structure-raygen.slangtests/spirv/descriptor-heap-acceleration-structure.slang
|
@coderabbitai review |
✅ Action performedReview finished.
|
jkwak-work
left a comment
There was a problem hiding this comment.
Looks good to me
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Verdict: ✅ Clean — no significant issues found
The PR fixes a stride/load type mismatch in the SPIR-V DescriptorHandle<RaytracingAccelerationStructure> heap path: the runtime-array element switches from OpTypeAccelerationStructureKHR to uint64, the default ArrayStride becomes a literal 8 (matching the OpLoad %u64 materialization), and -spirv-resource-heap-stride continues to override. The new getAccelerationStructureDescriptorRuntimeArrayType() helper, the getDescriptorRuntimeArrayType guarding SLANG_ASSERT, the outIsBufferResource = false write on the AS early-return, and the per-pass test split into default-stride CHECK and override-stride OVERRIDE runs are all internally consistent.
Changes Overview
SPIR-V emitter — AS descriptor heap stride fix (source/slang/slang-emit-spirv.cpp)
- New
getAccelerationStructureDescriptorRuntimeArrayType()builds anOpTypeRuntimeArrayofuint64with literalArrayStride 8by default; honors-spirv-resource-heap-strideoverride (cached in newm_descriptorHeapAccelerationStructureRuntimeArrayTypefield). getDescriptorHeapBaseTypererouteskIROp_RaytracingAccelerationStructureTypeto the new helper and writes*outIsBufferResource = false.getDescriptorRuntimeArrayTypeaddsSLANG_ASSERTthat AS opcode never reaches the legacy path.
CLI option help text (source/slang/slang-options.cpp)
-spirv-resource-heap-stridedescription extended to document the AS-specific literal-8 default.
Documentation (docs/command-line-slangc-reference.md, docs/user-guide/03-convenience-features.md, docs/user-guide/08-compiling.md)
- Default-stride wording updated;
RaytracingAccelerationStructurenote now states that the heap entry type isuint64_tand the default stride reflects the address width rather than the opaque AS type.
Tests (tests/spirv/descriptor-heap-acceleration-structure.slang, tests/spirv/descriptor-heap-acceleration-structure-raygen.slang)
- Each file now runs twice: a default-stride
CHECKrun pinningOpTypeRuntimeArray %[[U64]]+ArrayStride 8, and anOVERRIDErun with-spirv-resource-heap-stride 32pinningArrayStride 32. RayQuery and RayTracingKHR capability paths are both exercised.
jkwak-work
left a comment
There was a problem hiding this comment.
Looks good to me
Fixes #11231
Summary of the problem from the end user perspective
DescriptorHandle<RaytracingAccelerationStructure>heap loads could use a runtime-array stride based on the opaque acceleration structure type while loading a 64-bit device address. Nonzero heap indices could therefore address the wrong heap entry unless users pinned-spirv-resource-heap-stride.Minimal repro shader; if applicable
Root cause
The descriptor heap base type for acceleration structures was
OpTypeAccelerationStructureKHR, but the descriptor heap load materializes the value by loadinguint64and converting it withOpConvertUToAccelerationStructureKHR. That made the access-chain stride and load width disagree.Solution in this PR
Use a
uint64_truntime-array element for acceleration-structure heap entries and give it a defaultArrayStride 8, while still honoring an explicit-spirv-resource-heap-strideoverride. The regression tests now cover the unpinned default path for both RayQueryKHR and RayTracingKHR, plus explicit override precedence for acceleration-structure heap entries.Notes to the reviewers; where to focus on
The main behavior change is in
getDescriptorHeapBaseTypeandgetAccelerationStructureDescriptorRuntimeArrayType: AS heap entries take the literal 8-byte default stride becauseArrayStrideIdEXTis only valid for descriptor-typed arrays. Existing texture, sampler, texel-pointer, and buffer descriptor heap paths keep their prior stride behavior.Related PRs in the past