[SPIR-V] Add descriptor heap -fvk-resource-heap-stride / -fvk-sampler-heap-stride CLI flags

[Jonathan Zakharov (jzakharovnv)]

Building off of #8518, this PR adds two new command-line flags that override the ArrayStride of the descriptor heap runtime arrays emitted by -fspv-use-descriptor-heap. It is part 3/4 in a series.

-fvk-resource-heap-stride and -fvk-sampler-heap-stride sets the stride for ResourceDescriptorHeap SamplerDescriptorHeap arrays respectively. N and M must be a power of two in [8, 256]. When set, the CLI value takes the highest precedence.

Assisted by an AI agent.

Diego Novillo (@dnovillo)

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[Jonathan Zakharov (jzakharovnv)]

@microsoft-github-policy-service agree company="NVIDIA"

[Diego Novillo (dnovillo)]

These spec constant attributes are actually in #8520, right? Maybe move them there? This way, this PR just handles the flags.

[Jonathan Zakharov (jzakharovnv)]

Yes, this was a bit clumsy on my part. Will fix the documentation to remove mention of (now dropped feature) stride spec const attributes.

[Diego Novillo (dnovillo)]

If both flags are wrong, this will short-circuit and only give a single error message. Could you make both calls always happen and return the aggregate? Something like this:

bool ok = handleHeapStride(...resource...);
ok &= handleHeapStride(...sampler...);
if (!ok) return 1;

[Diego Novillo (dnovillo)]

Thanks for the fixes. LGTM now.

[Diego Novillo (dnovillo)]

Added an analysis on this series: #8517 (comment)

[Chris B (llvm-beanz)]

This string matching stuff is really gross. I posted a PR to clean this all up across the SPIRV code generator:
#8755

[Jonathan Zakharov (jzakharovnv)]

Agreed, thanks! Also I'm looking into addressing your comments for #8518 now.

[Copilot]

Pull request overview

Copilot reviewed 64 out of 64 changed files in this pull request and generated no new comments.

Suppressed comments (5)

tools/clang/unittests/SPIRV/SpirvContextTest.cpp:1

• Casting small integers to pointers is implementation-defined and can be brittle on platforms with pointer tagging or unusual address spaces. Prefer using addresses of real dummy objects (e.g., two distinct static locals) cast to SpirvInstruction * to preserve the intent (unique identity) without relying on integer-to-pointer conversions.
  tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.array-stride.hlsl:1
• Fix grammar in comment: 'it's' should be 'its' (possessive).
  tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.image-alias-fn-param.hlsl:1
• A blanket XFAIL: * can hide regressions indefinitely. If possible, scope the XFAIL to the specific known failing configuration(s) or add a follow-up tracking reference and an expiration plan (e.g., targeted XFAIL + TODO with issue/PR reference) so it’s clear when this should be removed.
  tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.typed-formats.hlsl:1
• These newly added comment lines include trailing whitespace. Please strip trailing spaces to match typical LLVM/Clang style and reduce diff noise in future edits.
  tools/clang/lib/SPIRV/SpirvEmitter.cpp:8647
• isHeapSourcedValue() does not treat function-scope aliases registered via DeclResultIdMapper::registerFnVarAlias() (used for RaytracingAccelerationStructure) as heap-sourced. This can misclassify RaytracingAccelerationStructure b = a; (where a is heap-loaded) as non-heap, potentially recording b as Bound in descriptorHeapVarState and causing false-positive 'mixing bound and descriptor heap resources' diagnostics later. Consider extending this helper to account for AS aliases (e.g., declIdMapper.hasFnVarAlias(var) for AS-typed vars, or tracking AS heap-init explicitly in descriptorHeapVarState and consulting that here).

bool SpirvEmitter::isHeapSourcedValue(const Expr *expr) const {
  if (isDescriptorHeap(expr->IgnoreParenCasts()))
    return true;
  const auto *var = dyn_cast_or_null<VarDecl>(getReferencedDef(expr));
  if (!var)
    return false;
  return descriptorHeapImageAliasVars.count(var) ||
         descriptorHeapBufferAliasVars.count(var);
}

[Copilot]

Pull request overview

Copilot reviewed 64 out of 64 changed files in this pull request and generated 1 comment.

Suppressed comments (6)

tools/clang/unittests/SPIRV/SpirvContextTest.cpp:1

• Using small integer values cast to pointers can trip UB/sanitizer checks on some platforms/configurations, even if the pointers are never dereferenced. To keep the test sanitizer-friendly while still validating pointer-identity uniquing, prefer using stable, non-null addresses (e.g., addresses of suitably aligned dummy storage or real instruction objects created via the builder) as stand-ins for distinct SpirvInstruction* identities.
  tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.array-stride.hlsl:1
• Fix grammar in the comment: 'it's' (it is) should be 'its' (possessive).
  tools/clang/include/clang/SPIRV/SpirvContext.h:94
• The hash for RuntimeArrayType does not incorporate the literal stride value when getStride().hasValue() is true, which can create many avoidable hash collisions (all literal strides hash the same). Include the stride value (when present) in getHashValue() (and keep arrayStrideId as part of the hash) to reduce collisions and improve lookup performance.

  static unsigned getHashValue(const RuntimeArrayType *Val) {
    return llvm::hash_combine(Val->getElementType(),
                              Val->getStride().hasValue(),
                              Val->getArrayStrideId());
  }

tools/clang/lib/SPIRV/SpirvEmitter.cpp:5352

• srcIsHeap only recognizes direct heap subscripts or alias variables. As a result, assignments that are still heap-only but wrapped in an expression (e.g., conditional selecting between two heap subscripts, parentheses beyond IgnoreParenCasts, etc.) can be misclassified as non-heap and trigger the 'mixing bound and descriptor heap resources' error, which is misleading. Consider extending isHeapSourcedValue() to recognize additional heap-only expression forms (notably conditional ?: where both arms are heap-sourced), or emitting a distinct diagnostic for 'unsupported heap-alias assignment expression' when the RHS contains heap accesses but isn't representable by the alias mechanism.

  const bool srcIsHeap = isHeapSourcedValue(srcExpr->IgnoreParenCasts());
  const bool wasHeap = descriptorHeapImageAliasVars.count(dstVar) ||
                       descriptorHeapBufferAliasVars.count(dstVar) ||
                       (stateIt != descriptorHeapVarState.end() &&
                        stateIt->second == DescriptorHeapVarState::Heap);

tools/clang/lib/SPIRV/SpirvEmitter.cpp:5364

• srcIsHeap only recognizes direct heap subscripts or alias variables. As a result, assignments that are still heap-only but wrapped in an expression (e.g., conditional selecting between two heap subscripts, parentheses beyond IgnoreParenCasts, etc.) can be misclassified as non-heap and trigger the 'mixing bound and descriptor heap resources' error, which is misleading. Consider extending isHeapSourcedValue() to recognize additional heap-only expression forms (notably conditional ?: where both arms are heap-sourced), or emitting a distinct diagnostic for 'unsupported heap-alias assignment expression' when the RHS contains heap accesses but isn't representable by the alias mechanism.

  if (mixingDetected) {
    emitError("mixing bound and descriptor heap resources in the same variable "
              "is not supported with SPV_EXT_descriptor_heap",
              loc);

tools/clang/lib/SPIRV/SpirvEmitter.cpp:9552

• getDescriptorHeapRuntimeArrayType() already receives onSamplerHeap, but the computed-stride selection is based on isa<SamplerType>(elemType) instead. This makes the behavior less explicit and can become fragile if the set of sampler-heap element types ever expands or if callers accidentally pass a non-sampler elemType with onSamplerHeap=true. Prefer selecting the stride via onSamplerHeap for clarity and consistency with the CLI-stride branch.

  SpirvInstruction *strideId = isa<SamplerType>(elemType)
                                   ? spvBuilder.getSamplerHeapArrayStride()
                                   : spvBuilder.getResourceHeapArrayStride();
  return spvContext.getRuntimeArrayType(elemType, llvm::None, strideId);

[Copilot]

Pull request overview

Copilot reviewed 64 out of 64 changed files in this pull request and generated 2 comments.

Suppressed comments (3)

tools/clang/test/CodeGenSPIRV/sm6_6.descriptorheap.ext.image-alias-fn-param.hlsl:1

• Lit will treat every // XFAIL: line as a directive. // XFAIL: heap image alias passed ... is not a valid XFAIL condition and will likely break test expectations (or stop the test from being XFAIL’ed). Keep only the directive (XFAIL: *), and change the explanatory line to a non-directive comment prefix (e.g. // NOTE:) so it doesn't get parsed by the test runner.
  tools/clang/unittests/SPIRV/SpirvContextTest.cpp:1
• The unit test uses integer-to-pointer reinterpret casts to fabricate distinct SpirvInstruction* values. This is implementation-defined and can be problematic under sanitizers or platforms with unusual pointer representations. Prefer using the addresses of real objects (e.g., two local dummy objects with stable addresses) to create distinct pointer identities without relying on integer->pointer casts.
  lib/DxcSupport/HLSLOptions.cpp:389
• The new tests in sm6_6.descriptorheap.ext.stride-cli-permute.hlsl expect the diagnostic text to end at ... (inclusive) (no trailing ; got <value>). Either update the tests’ BADRS/BADSS check lines to include the ; got <value> suffix, or adjust the diagnostic here to match the expected message format so the tests don't fail on exact string matching.

  // Power of 2 in [8, 256] inclusive.
  if (number < 8 || number > 256 || (number & (number - 1)) != 0) {
    errors << name
           << " must be a power of 2 between 8 and 256 (inclusive); got "
           << value;
    return false;
  }

[Copilot]

Pull request overview

Copilot reviewed 64 out of 64 changed files in this pull request and generated no new comments.

Suppressed comments (5)

tools/clang/unittests/SPIRV/SpirvContextTest.cpp:1

• Casting small integers to pointers can trigger UB-sanitizer diagnostics and is generally non-portable even if never dereferenced. Prefer using two distinct real addresses (e.g., addresses of two suitably-aligned dummy objects/byte buffers) to test pointer-identity uniquing without relying on integer-to-pointer conversions.
  tools/clang/lib/SPIRV/SpirvEmitter.cpp:2137
• There are two identical isRaytracingAccelerationStructure(decl->getType()) blocks back-to-back; the second block is unreachable due to the return true in the first. This should be removed to avoid dead code and to ensure future edits don’t accidentally diverge the behavior between the two copies.

  if (isRaytracingAccelerationStructure(decl->getType())) {
    if (SpirvInstruction *initVal = loadIfGLValue(init)) {
      declIdMapper.registerFnVarAlias(decl, initVal);
      // Track the AS as heap-initialized so diagnoseDescriptorHeapAliasMixing
      // can detect any later reassignment (even heap-to-heap, since
      // registerFnVarAlias cannot be updated after the fact).
      descriptorHeapVarState[decl] = DescriptorHeapVarState::Heap;
    } else {
      emitError("cannot create descriptor heap acceleration structure alias "
                "from initializer",
                init->getExprLoc());
    }
    return true;
  }

  if (isRaytracingAccelerationStructure(decl->getType())) {
    if (auto *initVal = loadIfGLValue(init))
      declIdMapper.registerFnVarAlias(decl, initVal);
    else
      emitError("cannot create descriptor heap acceleration structure alias "
                "from initializer",
                init->getExprLoc());
    return true;
  }

tools/clang/lib/SPIRV/SpirvEmitter.cpp:85

• This helper is declared at file scope with external linkage. It should be given internal linkage (e.g., static or moved into an anonymous namespace) to avoid leaking a generic symbol name from this TU and to prevent potential ODR/symbol collisions.

bool shaderModelKindIsRayTracing(hlsl::ShaderModel::Kind k) {
  switch (k) {
  case hlsl::ShaderModel::Kind::RayGeneration:
  case hlsl::ShaderModel::Kind::Intersection:
  case hlsl::ShaderModel::Kind::AnyHit:
  case hlsl::ShaderModel::Kind::ClosestHit:
  case hlsl::ShaderModel::Kind::Miss:
  case hlsl::ShaderModel::Kind::Callable:
    return true;
  default:
    return false;
  }
}

tools/clang/lib/SPIRV/SpirvEmitter.cpp:3579

• This branch explicitly avoids returning nullptr because it can propagate and crash before the diagnostic surfaces, but it still returns nullptr for void return types. If downstream callers don’t consistently treat nullptr as a hard failure for calls, this reintroduces the same crash risk. Prefer returning a non-null placeholder instruction for void calls as well (or ensure the call-lowering pipeline reliably short-circuits on nullptr everywhere).

        QualType retTy = callExpr->getCallReturnType(astContext);
        if (retTy->isVoidType())
          return nullptr;
        return spvBuilder.getUndef(retTy);

tools/clang/include/clang/SPIRV/SpirvContext.h:94

• RuntimeArrayType hashing still does not include the literal stride value when getStride().hasValue() is true, so all runtime arrays with different literal strides (but same element type) collide into the same hash bucket. With the expanded use of runtime arrays for descriptor heaps, this can become a measurable DenseSet/DenseMap bottleneck. Include the stride value (when present) in the hash to reduce collisions.

  static unsigned getHashValue(const RuntimeArrayType *Val) {
    return llvm::hash_combine(Val->getElementType(),
                              Val->getStride().hasValue(),
                              Val->getArrayStrideId());
  }