Add opt-in unified ArrayStride for SPIR-V resource descriptor heaps#11723
Conversation
jkwak-work
left a comment
There was a problem hiding this comment.
@nv-slang-bot , Fix the format workflow problem.
- Simplify the getUnifiedResourceHeapStride and conflict-diagnose doc comments (drop the spirv-val error-case detail) per review; this also clears the Check Formatting failure, since the comments are now shorter. - Remove the redundant call-site comment in emitSPIRVFromIR. - Add tests/spirv/descriptor-heap-unified-stride-runtime.slang: a -vk bindless COMPARE_COMPUTE exercising descriptor-handle resources at runtime. - Tighten the option doc/help wording to image and buffer descriptor sizes.
|
Thanks for the review @jkwak-work — addressed in c2dc734:
🤖 Generated by an automated Slang coworker — may be inaccurate. A human maintainer should verify. |
Remove tests/spirv/descriptor-heap-unified-stride-runtime.slang: @jkwak-work retracted the runtime-test request because the runtime stack (slang-rhi) does not yet support VK_EXT_descriptor_heap, so a COMPARE_COMPUTE cannot exercise the unified-stride path. The feature stays statically verified.
jkwak-work
left a comment
There was a problem hiding this comment.
It looks good to me.
And I think the code properly addressed what Yong concerned about.
jkwak-work
left a comment
There was a problem hiding this comment.
@nv-slang-bot , don't we need to update the documents for the new option?
Add the new option to the three user-facing doc surfaces, matching the wording of the sibling -spirv-resource-heap-stride / -spirv-sampler-heap-stride options: - docs/user-guide/03-convenience-features.md: prose paragraph in the spvDescriptorHeapEXT section explaining the shared image-vs-buffer-max stride, the default-path-only scope, mutual exclusivity with -spirv-resource-heap-stride, and that the sampler heap and acceleration-structure entries are unaffected. - docs/user-guide/08-compiling.md: SPIRVUnifiedDescriptorHeapStride row in the compilation-API options table. - docs/command-line-slangc-reference.md: regenerated via `slangc -help-style markdown -h`, adding the generated -spirv-unified-descriptor-heap-stride block.
…stride (#11723) diagnoseConflictingDescriptorHeapStrideOptions() raises E57006 only when SPIRVResourceHeapStride != 0; an explicit `-spirv-resource-heap-stride 0` selects the same default OpConstantSizeOfEXT path that the unified option modifies and is accepted. Correct the option help text, the slang.h enumerator doc comment, and all three user-facing doc surfaces to say the unified option is mutually exclusive with a non-zero -spirv-resource-heap-stride (combining the two is the error), rather than overstating that supplying the resource-heap-stride option at all conflicts.
…11723) Match the comment to the actual condition: the conflict is between a non-zero explicit -spirv-resource-heap-stride and the unified stride; an explicit stride of 0 selects the same default OpConstantSizeOfEXT path the unified option modifies and is not a conflict.
Second internal comment that overstated the conflict: clarify that unified stride only affects the auto path (when -spirv-resource-heap-stride is 0), the conflict is with a non-zero -spirv-resource-heap-stride, and an explicit 0 selects that same auto path and is not a conflict.
Comment-only: the CONFLICT variant uses -spirv-resource-heap-stride 64, and the conflict is specifically with a non-zero explicit stride (explicit 0 selects the same default path and is accepted). Test directives are unchanged.
|
@jkwak-work Done — documented
While writing the docs I also tightened the conflict wording everywhere (option help text, 🤖 Generated by an automated Slang coworker — may be inaccurate. A human maintainer should verify. |
) Per review (@jkwak-work): diagnose the `-spirv-unified-descriptor-heap-stride` / non-zero `-spirv-resource-heap-stride` mutual exclusivity in OptionsParser's post-parse validation (alongside ConflictingProfilesSpecifiedForTarget / ConflictingStagesForEntryPoint), reading both from linkage->m_optionSet, so E57006 fires as soon as the options are parsed -- before any SPIR-V emission and regardless of whether the module declares a resource heap. An explicit `-spirv-resource-heap-stride 0` selects the default OpConstantSizeOfEXT path the unified option modifies and is accepted. Keep diagnoseConflictingDescriptorHeapStrideOptions() at emit time as the compile-API safety net: SPIRVResourceHeapStride / SPIRVUnifiedDescriptorHeapStride are public API options, and entries set directly through the compile API do not flow through OptionsParser, so without the emit-time check that path would silently honor the explicit stride. The two do not double-diagnose: an E57006 at parse time aborts before emission, so the emit-time check only fires for the API path. CONFLICT test variant still produces error[E57006]; build + 4/4 static tests + spirv-val EXIT=0 for both entries green.
clang-format 17.0.6 reflows the two doc comments touched in c71ee1b (the isUnifiedResourceHeapStrideEnabled and diagnoseConflictingDescriptorHeapStrideOptions comments). Match its wrapping exactly; comment text unchanged.
Add the SPIRVUnifiedDescriptorHeapStride option (-spirv-unified-descriptor-heap-stride): when set, every resource descriptor-heap runtime array advertises one shared ArrayStride equal to the maximum of every per-type OpConstantSizeOfEXT, built as an OpSpecConstantOp Select/UGreaterThan chain and applied via OpDecorateId ArrayStrideIdEXT. A heap shared by multiple descriptor types then maps a descriptor handle to the same byte offset regardless of which type's runtime array is indexed. The deferred maximum is emitted in a finalize pass and each participating array is moved after it so the shared stride id precedes the target array type, satisfying the spirv-tools ArrayStrideIdEXT operand-ordering rule. Explicit -spirv-resource-heap-stride still wins by construction; samplers (separate heap) and acceleration structures (non-zero stride) are excluded. Fixes #11718.
Rework the opt-in unified ArrayStride so it is the canonical fixed sequence max(sizeof(image descriptor), sizeof(buffer descriptor)), emitted regardless of which resource descriptor types a shader actually references. The host packs one resource heap at this device-defined stride, so two shaders that share the heap must compute an identical stride or they would mis-index; an adaptive (per-shader) stride breaks that invariant. Samplers live in a separate heap, so the resource-heap maximum folds only image vs buffer. - getUnifiedResourceHeapStride() memoizes the fixed Select chain max(a,b)=Select(UGreaterThan(a,b),a,b) over OpConstantSizeOfEXT of a placeholder image and a buffer descriptor; emitted lazily before the first resource array so its <id> precedes the ArrayStrideIdEXT use. - The placeholder image uses %float / Sampled=1 rather than the proposal's %void / Sampled=0: a descriptor's device-defined size is its heap-slot size, independent of the image's sampled type/flag, and %void/Sampled=0 fails Vulkan spirv-val (VUID-StandaloneSpirv- OpTypeImage-04656/04657). - Diagnose combining -spirv-unified-descriptor-heap-stride with an explicit -spirv-resource-heap-stride as a hard error (E57006); the two are mutually exclusive.
- Simplify the getUnifiedResourceHeapStride and conflict-diagnose doc comments (drop the spirv-val error-case detail) per review; this also clears the Check Formatting failure, since the comments are now shorter. - Remove the redundant call-site comment in emitSPIRVFromIR. - Add tests/spirv/descriptor-heap-unified-stride-runtime.slang: a -vk bindless COMPARE_COMPUTE exercising descriptor-handle resources at runtime. - Tighten the option doc/help wording to image and buffer descriptor sizes.
Remove tests/spirv/descriptor-heap-unified-stride-runtime.slang: @jkwak-work retracted the runtime-test request because the runtime stack (slang-rhi) does not yet support VK_EXT_descriptor_heap, so a COMPARE_COMPUTE cannot exercise the unified-stride path. The feature stays statically verified.
Add the new option to the three user-facing doc surfaces, matching the wording of the sibling -spirv-resource-heap-stride / -spirv-sampler-heap-stride options: - docs/user-guide/03-convenience-features.md: prose paragraph in the spvDescriptorHeapEXT section explaining the shared image-vs-buffer-max stride, the default-path-only scope, mutual exclusivity with -spirv-resource-heap-stride, and that the sampler heap and acceleration-structure entries are unaffected. - docs/user-guide/08-compiling.md: SPIRVUnifiedDescriptorHeapStride row in the compilation-API options table. - docs/command-line-slangc-reference.md: regenerated via `slangc -help-style markdown -h`, adding the generated -spirv-unified-descriptor-heap-stride block.
…stride (#11723) diagnoseConflictingDescriptorHeapStrideOptions() raises E57006 only when SPIRVResourceHeapStride != 0; an explicit `-spirv-resource-heap-stride 0` selects the same default OpConstantSizeOfEXT path that the unified option modifies and is accepted. Correct the option help text, the slang.h enumerator doc comment, and all three user-facing doc surfaces to say the unified option is mutually exclusive with a non-zero -spirv-resource-heap-stride (combining the two is the error), rather than overstating that supplying the resource-heap-stride option at all conflicts.
…11723) Match the comment to the actual condition: the conflict is between a non-zero explicit -spirv-resource-heap-stride and the unified stride; an explicit stride of 0 selects the same default OpConstantSizeOfEXT path the unified option modifies and is not a conflict.
Second internal comment that overstated the conflict: clarify that unified stride only affects the auto path (when -spirv-resource-heap-stride is 0), the conflict is with a non-zero -spirv-resource-heap-stride, and an explicit 0 selects that same auto path and is not a conflict.
Comment-only: the CONFLICT variant uses -spirv-resource-heap-stride 64, and the conflict is specifically with a non-zero explicit stride (explicit 0 selects the same default path and is accepted). Test directives are unchanged.
) Per review (@jkwak-work): diagnose the `-spirv-unified-descriptor-heap-stride` / non-zero `-spirv-resource-heap-stride` mutual exclusivity in OptionsParser's post-parse validation (alongside ConflictingProfilesSpecifiedForTarget / ConflictingStagesForEntryPoint), reading both from linkage->m_optionSet, so E57006 fires as soon as the options are parsed -- before any SPIR-V emission and regardless of whether the module declares a resource heap. An explicit `-spirv-resource-heap-stride 0` selects the default OpConstantSizeOfEXT path the unified option modifies and is accepted. Keep diagnoseConflictingDescriptorHeapStrideOptions() at emit time as the compile-API safety net: SPIRVResourceHeapStride / SPIRVUnifiedDescriptorHeapStride are public API options, and entries set directly through the compile API do not flow through OptionsParser, so without the emit-time check that path would silently honor the explicit stride. The two do not double-diagnose: an E57006 at parse time aborts before emission, so the emit-time check only fires for the API path. CONFLICT test variant still produces error[E57006]; build + 4/4 static tests + spirv-val EXIT=0 for both entries green.
clang-format 17.0.6 reflows the two doc comments touched in c71ee1b (the isUnifiedResourceHeapStrideEnabled and diagnoseConflictingDescriptorHeapStrideOptions comments). Match its wrapping exactly; comment text unchanged.
At merge time this PR collided with the concurrently-queued #11556, which appends CompilerVersion = 153 to CompilerOptionName. Both PRs branched when the enum tail was TraceCoverageBoolean = 152 and each took the next free value 153, producing a `duplicate case value` build break in the OptionKind switch (slang-options.cpp) once combined in a merge group. Yield 153 to #11556's CompilerVersion and append this option at 154 instead. Append-only, so no existing enumerator value shifts (ABI-safe). The two now hold distinct values regardless of merge order.
3a48114
5d6a68e to
3a48114
Compare
|
Rebased onto current This PR's head CI was green on its own, but it was evicted from the merge queue by a Fix: yield 🤖 Generated by an automated Slang coworker — may be inaccurate. A human maintainer should verify. |
|
@jkwak-work — courtesy heads-up: the force-push for the rebase auto-dismissed your prior approval, so this PR is back at The only change since you approved is the verified 🤖 Generated by an automated Slang coworker — may be inaccurate. A human maintainer should verify. |
Motivation
Fixes #11718.
VK_EXT_descriptor_heap/spvDescriptorHeapEXTpacks every resource descriptor(images and buffers) into one runtime array — the "resource heap" — and a separate
array for samplers. The host allocator stores each heap slot at a fixed stride:
for the resource heap, the spec (§"Shader Model 6.6 - SamplerHeap and ResourceHeap")
defines that stride as the canonical maximum
so that
index * stridelands on the same byte offset regardless of which descriptorcategory a given access happens to read.
Slang previously emitted a per-type
ArrayStrideIdEXTderived only from the descriptortypes a single shader referenced. That is wrong for a shared heap: an image-only
shader and a buffer-only shader, indexing the same host-packed heap, would advertise
two different strides and mis-index each other's descriptors. The motivating case:
Compiled with
-spirv-unified-descriptor-heap-stride, both resource arrays here — and abuffer-only shader sharing the heap — must advertise the identical image-vs-buffer max.
Proposed solution
Add an opt-in option,
-spirv-unified-descriptor-heap-stride, that decorates everyimage/buffer resource descriptor-heap runtime array with one shared, fixed
ArrayStrideIdEXT:the canonical
max(sizeof(image descriptor), sizeof(buffer descriptor)). (Acceleration-structureheap entries are unaffected — they keep their existing literal
uint64stride.)The stride is not adaptive to the shader's contents. Because the host heap layout is
independent of any one shader, the emitted stride is always the full image-vs-buffer max —
even for a shader that touches only a buffer. A buffer-only shader still emits the
placeholder image descriptor type and the full
Selectchain, so two shaders over the sameheap can never disagree.
OpConstantSizeOfEXTis device-defined (the descriptor size is decided by the driver,not the compiler), so the maximum cannot be a compile-time literal. It is expressed
symbolically as
max(a, b) = Select(UGreaterThan(a, b), a, b)usingOpSpecConstantOp,whose result is a constant
<id>— a requirement forArrayStrideIdEXT.(
OpExtInst GLSL.std.450 UMaxwill not do: its result is not a constant<id>.)The unified stride is built lazily and memoized the first time a resource array needs it
(
getUnifiedResourceHeapStride()), so the constant<id>is emitted before the firstarray type —
ArrayStrideIdEXTrequires the stride<id>to precede the array it decorates.There is no separate finalize pass and no array reordering.
Samplers are out of scope. The spec uses two heaps — samplers are never co-located with
resources — so the unified resource-heap stride folds only image-vs-buffer. Sampler arrays
(
OpTypeSamplerelement type) are excluded from the fold and keep their existing-spirv-sampler-heap-stridebehavior.Mutual exclusivity. A non-zero
-spirv-resource-heap-strideand the unified maximumstride are mutually exclusive — the explicit literal and the device-defined max are different
contracts for the same array, so combining them is a hard error (
E57006,SpirvConflictingDescriptorHeapStrideOptions), not a silent "one wins." An explicit-spirv-resource-heap-stride 0selects the same defaultOpConstantSizeOfEXTpath the unifiedoption modifies and is therefore accepted (not a conflict).
Placeholder image descriptor type — deviation from the proposal
The issue's literal proposal sketches the placeholder image descriptor as
OpTypeImage %void 2D 2 0 0 0 Unknown. That is invalid SPIR-V for Vulkan:VUID-StandaloneSpirv-OpTypeImage-04656— the sampled type must be a 32-bit int,64-bit int, or 32-bit float scalar;
%voidis rejected.VUID-StandaloneSpirv-OpTypeImage-04657—Sampledmust be1or2, never0.A descriptor's device-defined size is its heap-slot footprint and is independent of the
image's sampled type and
Sampledflag, so we use a spec-valid placeholderOpTypeImage %float 2D 2 0 0 1 Unknown(sampled typefloat,Sampled = 1; depth operandstays
Unknown = 2). Both entry points of the regression test then pass static SPIR-Vvalidation (see below). cc @csyonghe — flagging the placeholder deviation explicitly.
Change summary
include/slang.hSPIRVUnifiedDescriptorHeapStride = 153before theCountOfsentinel (additive, ABI-safe); doc notes mutual exclusivity with a non-zero-spirv-resource-heap-stride.source/slang/slang-options.cpp-spirv-unified-descriptor-heap-strideand add its case to the boolean-option parse switch.source/slang/slang-emit-spirv.cppgetUnifiedResourceHeapStride()lazily emits the fixed image-vs-bufferSelectmax (memoized).getDescriptorRuntimeArrayType()decorates resource arrays with that sharedArrayStrideIdEXTwhen the option is on; sampler arrays are excluded.diagnoseConflictingDescriptorHeapStrideOptions()raisesE57006when the unified option and a non-zero-spirv-resource-heap-strideare both set.source/slang/slang-diagnostics.luaSpirvConflictingDescriptorHeapStrideOptions(E57006).tests/spirv/descriptor-heap-unified-stride.slangSelect), CONFLICT (diagnostic — unified option + a non-zero-spirv-resource-heap-stride→E57006), SINGLE (buffer-only shader still emits the full image-vs-buffer max, proving non-adaptivity).Concepts and vocabulary
spvDescriptorHeapEXTuses two separate runtimearrays. The unified stride applies only to the resource heap (images + buffers).
ArrayStrideIdEXT— decorates a runtime array with a constant<id>byte stride; that<id>must be defined before the array type in the module.OpConstantSizeOfEXT— a device-defined (opaque, driver-decided) constant giving adescriptor type's heap-slot size. Not a compile-time literal.
image-vs-buffer max, never on which descriptor types the shader happens to reference.
Process report
include/slang.h— new options need aCompilerOptionNameenumerator. AppendedSPIRVUnifiedDescriptorHeapStride = 153immediately beforeCountOfwith an explicitvalue (never mid-enum), so existing ABI offsets are unchanged.
pr: non-breaking.source/slang/slang-options.cpp— the option is a pure boolean (presence = on), so itis registered with a
nullptrvalue-spec and routed through the existing boolean-optionparse case alongside
TraceCoverageBoolean/SkipSPIRVValidation. No new parsingmachinery.
source/slang/slang-emit-spirv.cpp—getUnifiedResourceHeapStride()builds, inConstantsAndTypes, the placeholder imagedescriptor type (
OpTypeImage %float 2D 2 0 0 1 Unknown), the buffer descriptor type(
ensureDescriptorHeapBufferDescriptorType(SpvStorageClassUniform)), their twoOpConstantSizeOfEXT,OpSpecConstantOp OpUGreaterThan, andOpSpecConstantOp OpSelect.It is memoized on
m_unifiedResourceHeapStrideand emitted lazily so the stride<id>precedes the first decorated array (the
ArrayStrideIdEXTordering rule).getDescriptorRuntimeArrayType()gates the fold onarrayStride == 0 && isResourceElement && isUnifiedResourceHeapStrideEnabled(), whereisResourceElement = descriptorElementType->opcode != SpvOpTypeSampler. When folding, itskips the per-type
OpConstantSizeOfEXTand decorates with the sharedemitOpDecorateArrayStrideIdEXT; otherwise it keeps the prior per-type / literal path.Input-shape check: the per-array descriptor element type is the correct, principled
input — the fold is a property of the heap, not of a malformed type, so it lives at
the array-construction site rather than masking anything upstream.
diagnoseConflictingDescriptorHeapStrideOptions()is called once inemitSPIRVFromIR(after the forward-declared-pointers loop) and raises
E57006when the unified optionand a non-zero
-spirv-resource-heap-strideare both set. It fires on the CLIcombination even when the module declares no resource heap (the user supplied
contradictory intent), and emits no instructions.
source/slang/slang-diagnostics.lua—SpirvConflictingDescriptorHeapStrideOptions(
E57006) is a location-less hard error: "an explicit resource heap stride and the unifiedmaximum stride are mutually exclusive." It is only emitted when
-spirv-resource-heap-strideis non-zero (an explicit
0is the default path and is accepted), so the "explicit resourceheap stride" it names is always a non-zero one.
tests/spirv/descriptor-heap-unified-stride.slang— the SINGLE variant is the keyguardrail: a buffer-only shader compiled with the flag still emits the placeholder
OpTypeImageand the fullSelectchain, demonstrating the stride is fixed, not adaptive.Validation
Built
slang-test(Debug);descriptor-heap-unified-stride.slangpasses all 4 static variants(UNIFIED, DEFAULT, CONFLICT, SINGLE).
Static SPIR-V validation of both unified-stride entry points, under
SLANG_RUN_SPIRV_VALIDATION=1, exits 0:🤖 Generated by an automated Slang coworker — may be inaccurate. A human maintainer should verify.