Skip to content

Fix #11861: honor vk::binding on resource-containing struct params#11873

Merged
jkiviluoto-nv merged 3 commits into
masterfrom
fix/issue-11861
Jul 2, 2026
Merged

Fix #11861: honor vk::binding on resource-containing struct params#11873
jkiviluoto-nv merged 3 commits into
masterfrom
fix/issue-11861

Conversation

@nv-slang-bot

@nv-slang-bot nv-slang-bot Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Stacked on #11870 (base: fix/issue-11857). #11870 fixes the sibling issue #11857 by
removing the too-permissive PtrType leaf from isVkBindingCompatibleEntryPointParameterType.
This PR adds the struct recursion on top of that branch, so the recursion is faithful by
construction (a struct of only pointers correctly still warns). Review/merge #11870 first; this
PR's diff is only the struct-recursion delta. Once #11870 lands, I will rebase this onto master.

Motivation

Since #11712 ("Honor vk::binding on entry point parameters"), a [[vk::binding(set, binding)]]
annotation on an entry-point parameter whose type is a struct that contains resources is honored
by the binder (the fields are placed correctly) but is also diagnosed with warning
E38010 ("modifier on entry point parameter is unsupported ... will be ignored"). The diagnostic
contradicts the actual, correct behavior, and under -warnings-as-errors 38010 it rejects an
otherwise-valid shader.

Consider this shader:

struct Resources { Texture2D<float4> tex; SamplerState samp; };

[shader("compute")]
[numthreads(1,1,1)]
void main([[vk::binding(2, 1)]] uniform Resources resources) { ... }

The binder places resources.tex at (binding 2, set 1) and resources.samp at (binding 3, set 1)
— exactly as requested — yet E38010 fires on resources, falsely claiming the attribute was ignored.

Proposed solution

The regression is a disagreement between two predicates introduced/used by #11712 that must stay in
sync but don't:

  • The diagnostic predicate isVkBindingCompatibleEntryPointParameterType
    (source/slang/slang-check-shader.cpp) runs in validateEntryPoint before layout and approximates
    "will this parameter consume a descriptor slot?" by inspecting the AST type. It recognized the
    leaf resource types (and recursed through arrays / ModifiedType) but did not recurse into an
    aggregate struct, so a resource-containing struct returned false → the warning was not suppressed.
  • The binder (findVkBindingEntryPointParameterResourceInfo /
    hasSupportedVkBindingOnEntryPointParameter, source/slang/slang-parameter-binding.cpp) decides
    after layout by resource kind: it honors [[vk::binding]] whenever the parameter's computed type
    layout accumulates a DescriptorTableSlot or SubElementRegisterSpace. A struct-of-resources
    aggregates a DescriptorTableSlot from its fields, so the binder honors it.

Because the diagnostic approximates by AST type (did not decompose aggregates) while the binder reads the
computed layout (does decompose aggregates), the two diverge for a struct-of-resources: the binder
honors, the diagnostic warns.

The fix makes the AST predicate a faithful subset of the binder's honoring contract: an aggregate
struct is compatible iff at least one of its fields — transitively, and including fields inherited from
a base struct — is compatible. This is the principled layer for the fix: the diagnostic is an AST-type
approximation (it runs before any layout is available), so the correct repair is to make that
approximation decompose aggregates the same way the binder's layout does, rather than to mask the symptom
at the call site. Field types are substituted through the struct's DeclRef, so a generic field such as
T tex with T = Texture2D is still recognized; nested structs are handled by the recursive call.

Why this stacks on #11870 rather than standing alone. The struct recursion reuses the same leaf
predicate for each field. If that predicate still returned true for a raw pointer (uint*) — as it did
before #11857 was fixed — then struct { uint* p; } would be wrongly reported as compatible and its
E38010 warning suppressed, even though the binder never positions a pointer (a pointer is a
buffer-device-address value, not a descriptor slot). #11870 removes that PtrType leaf (the #11857 fix),
so building the struct recursion on top of it keeps the predicate a faithful subset by construction: a
struct of only pointers finds no descriptor-consuming leaf and correctly still warns.

Approach considered and deferred: deciding purely from the computed type layout (a single source of
truth shared by diagnostic and binder) would structurally prevent this class of divergence and would also
subsume the sibling issue #11857. It is left as possible future work because the diagnostic runs before
binding layout, so a type layout is not readily available there; consulting it would require computing an
on-demand layout in the checker or moving the diagnostic after layout, which is a larger, higher-risk
change. This PR takes the smaller, low-risk repair that matches the function's existing recursion style.

Change summary

  • source/slang/slang-check-shader.cpp
    • isVkBindingCompatibleEntryPointParameterType: take an ASTBuilder*; add a DeclRefType
      StructDecl case that returns true if any instance field (via getFields, with substituted field
      types via getType) is compatible, or if a base struct (via findBaseStructType) is. Thread the
      ASTBuilder* through the existing array / ModifiedType recursion.
    • Its single call site in validateEntryPoint now passes linkage->getASTBuilder().
  • tests/spirv/vk-binding-entry-point-param-struct.slang (new): struct-of-resources entry-point params
    with [[vk::binding]], asserting the requested Binding/DescriptorSet decorations and — via
    -warnings-as-errors 38010 — that no E38010 fires. Covers each distinct traversal the diff adds:
    flat, nested, single- and multi-level inheritance (the base recursion re-enters — the emitted
    %…_multi_base_base_tex name confirms two base hops), a generic struct field
    (Wrapper<Texture2D<float4>> — the substituted getType path the comment relies on, which nothing
    had exercised), a resource field that is not first (the field loop must continue past plain-data
    fields), and an array-of-struct (the array arm composed with the struct arm).
  • tests/diagnostics/vk-binding-entry-point-param-struct-no-resource.slang (new): a struct of only
    plain-data fields and a struct of only a raw-pointer field with [[vk::binding]] both still warn
    E38010 (guards against over-broadening the struct recursion — the pointer case is the vk::binding on pointer entry-point parameters is accepted but ignored #11857-class
    shape that the recursion must not suppress).

Concepts and vocabulary

  • Descriptor-shaped resource kind: the binder honors [[vk::binding]] only for a parameter whose
    layout consumes a DescriptorTableSlot (a plain texture/buffer/sampler) or SubElementRegisterSpace
    (a whole-space parameter such as a ParameterBlock); see isVkBindingEntryPointParameterResourceKind.
    The AST predicate's leaf cases are the type-level mirror of that set.
  • Pre-layout vs post-layout: isVkBindingCompatibleEntryPointParameterType runs in
    validateEntryPoint before any per-target layout exists, so it must reason about AST types;
    the binder runs after layout and reads TypeLayout::ResourceInfo. Keeping the former a faithful subset
    of the latter is the invariant this PR restores.

Process report

Why the struct-recursion change is necessary and correct (code trace). For
[[vk::binding(2,1)]] uniform Resources resources with
struct Resources { Texture2D tex; SamplerState samp; }, validateEntryPoint
(slang-check-shader.cpp) computes
isVkBindingCompatibleEntryPointParameterType(Resources). Before this change the function had no
DeclRefType/StructDecl case, so it fell through to return false; the call site then saw
supportsVkBindingOnParameter == false while the parameter did carry a GLSLBindingAttribute, and
emitted UnhandledModOnEntryPointParameter (E38010). Meanwhile
_generateParameterBindingshasSupportedVkBindingOnEntryPointParameter
(slang-parameter-binding.cpp) found a DescriptorTableSlot in Resources's type layout and placed
tex/samp at (2,1)/(3,1). After this change the predicate decomposes the struct: it iterates the
instance fields via getFields(astBuilder, structDeclRef, MemberFilterStyle::Instance), substitutes each
field type via getType(astBuilder, fieldDeclRef), and recurses; tex is a ResourceType → returns
true → the warning is suppressed, matching the binder. A struct of only plain-data fields finds no
compatible field (and no compatible base) and still returns false, so E38010 still fires for a truly
ignored annotation.

Is the input shape correct, or should a producer be fixed? The input shape (a resource-containing
struct entry-point parameter) is a genuine, valid, supported shape — the binder already honors it. The
defect is only that the diagnostic's pre-layout approximation was incomplete, so the fix belongs in that
approximation. No upstream producer emits a malformed type here.

Over-broadening / faithful-subset check. The struct case returns true only when a genuine
descriptor-consuming leaf is found transitively, so the predicate remains a subset of what the binder
honors — it cannot start suppressing the warning for a parameter the binder would ignore. This is why the
PR stacks on #11870: with the PtrType leaf removed (the #11857 fix), a struct whose only field is a raw
pointer finds no descriptor-consuming leaf and correctly still warns E38010, which the new negative test
asserts directly. Targets that do not support the feature
(supportsVkBindingOnEntryPointParameters == false, e.g. CUDA/Metal/C++) are unaffected and still warn.

Recursion termination (no cycle/depth guard, and why). The sibling struct-field walks in this file
(validateVaryingType, collectGenericStructTypeUses) carry visited-set + depth guards, so it is fair
to ask why this one does not. The reason is that every cycle that could make the recursion diverge is
rejected by a fatal front-end diagnostic before validateEntryPoint runs this predicate: a value-typed
struct cycle (struct S { S next; }) exceeds the nesting limit → E39997 (kMaxTypeNestingDepth = 128),
and a cyclic inheritance graph → E39999. So the recursion only ever descends a finite, acyclic
field/base structure. A visited-set would therefore be dead code under correct input (it can never fire),
which the methodology counsels against; the termination invariant is documented at the function instead.
The one residual — a pathologically deep non-cyclic inheritance chain — is a pre-existing
unbounded-recursion property shared by other base-walks in the compiler, not introduced by this change,
and inheritance is itself deprecated (E30816). Checked empirically: struct S { S next; } as a
uniform entry param exits cleanly with E39997, not a crash.

Coordination with #11857 / #11870. #11870 fixes #11857 (the PtrType leaf is too permissive) by
removing that leaf. This PR is stacked on #11870's branch so the struct recursion is faithful by
construction and there is no textual conflict; the two changes are intended to land together.

Fixes #11861.

@nv-slang-bot nv-slang-bot Bot added the pr: non-breaking PRs without breaking changes label Jul 1, 2026
nv-slang-bot Bot added a commit that referenced this pull request Jul 1, 2026
Address the peer-review nits on PR #11873 (all non-blocking; the struct
recursion itself is unchanged):

- Expand tests/spirv/vk-binding-entry-point-param-struct.slang with the
  recursion paths the diff introduces but the original test did not
  exercise: multi-level inheritance (the base recursion re-enters more
  than once), a generic struct field (the substituted getType path the
  comment relies on), a resource field that is not first (the field loop
  must continue past plain-data fields), and an array-of-struct (the array
  arm composed with the struct arm). Each asserts the requested
  Binding/DescriptorSet under -warnings-as-errors 38010.
- Document the recursion's termination invariant in
  isVkBindingCompatibleEntryPointParameterType: value-typed struct cycles
  and cyclic inheritance are rejected earlier by E39997/E39999, so the
  recursion descends a finite, acyclic structure and needs no cycle/depth
  guard.
- Note why MemberFilterStyle::Instance (instance-vs-static, not
  own-vs-inherited) makes the separate findBaseStructType branch necessary,
  and why static members are excluded.
- tests/diagnostics/vk-binding-entry-point-param-struct-no-resource.slang:
  drop the unnecessary 'non-exhaustive' directive (all diagnostics are
  matched by annotations; slang-test flags it as an error otherwise).
@jkiviluoto-nv jkiviluoto-nv marked this pull request as ready for review July 1, 2026 12:53
@jkiviluoto-nv jkiviluoto-nv requested a review from a team as a code owner July 1, 2026 12:53
jkiviluoto-nv
jkiviluoto-nv previously approved these changes Jul 1, 2026

@jkiviluoto-nv jkiviluoto-nv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated locally.

nv-slang-bot Bot added 2 commits July 2, 2026 00:03
…nt params

The pre-layout diagnostic predicate isVkBindingCompatibleEntryPointParameterType
in slang-check-shader.cpp approximates "will this parameter consume a descriptor
slot?" by AST type, but did not recurse into an aggregate struct. A struct that
CONTAINS resources (e.g. struct Resources { Texture2D tex; SamplerState samp; })
therefore returned false, so E38010 ("attribute ignored") was not suppressed --
even though the post-layout binder honors the annotation and places the fields.
Under -warnings-as-errors 38010 this rejected an otherwise-valid shader.

Make the AST predicate a faithful subset of the binder's contract: a struct is
compatible iff at least one of its fields -- transitively, and including fields
inherited from a base struct -- is compatible. Field types are substituted through
the struct's DeclRef so generic fields are recognized, and nested structs are
handled by the recursive call. Thread an ASTBuilder* through the existing array /
ModifiedType recursion for getFields/getType/findBaseStructType.

Stacked on #11870 (fixes #11857 by removing the too-permissive PtrType leaf), so
the struct recursion is a faithful subset by construction: a struct of only
pointers finds no descriptor-consuming leaf and still (correctly) warns E38010.

Tests:
- tests/spirv/vk-binding-entry-point-param-struct.slang: flat, nested, and
  inherited struct-of-resources params honored; no E38010 under -warnings-as-errors.
- tests/diagnostics/vk-binding-entry-point-param-struct-no-resource.slang: struct
  of only plain data and struct of only a raw pointer both still warn E38010.

Fixes #11861.
Address the peer-review nits on PR #11873 (all non-blocking; the struct
recursion itself is unchanged):

- Expand tests/spirv/vk-binding-entry-point-param-struct.slang with the
  recursion paths the diff introduces but the original test did not
  exercise: multi-level inheritance (the base recursion re-enters more
  than once), a generic struct field (the substituted getType path the
  comment relies on), a resource field that is not first (the field loop
  must continue past plain-data fields), and an array-of-struct (the array
  arm composed with the struct arm). Each asserts the requested
  Binding/DescriptorSet under -warnings-as-errors 38010.
- Document the recursion's termination invariant in
  isVkBindingCompatibleEntryPointParameterType: value-typed struct cycles
  and cyclic inheritance are rejected earlier by E39997/E39999, so the
  recursion descends a finite, acyclic structure and needs no cycle/depth
  guard.
- Note why MemberFilterStyle::Instance (instance-vs-static, not
  own-vs-inherited) makes the separate findBaseStructType branch necessary,
  and why static members are excluded.
- tests/diagnostics/vk-binding-entry-point-param-struct-no-resource.slang:
  drop the unnecessary 'non-exhaustive' directive (all diagnostics are
  matched by annotations; slang-test flags it as an error otherwise).
@nv-slang-bot nv-slang-bot Bot changed the base branch from fix/issue-11857 to master July 2, 2026 00:16
@nv-slang-bot nv-slang-bot Bot dismissed jkiviluoto-nv’s stale review July 2, 2026 00:16

The base branch was changed.

@nv-slang-bot nv-slang-bot Bot force-pushed the fix/issue-11861 branch from 5660a26 to fcdaab4 Compare July 2, 2026 00:17
The vk::binding entry-point-parameter loop hoisted
`auto astBuilder = linkage->getASTBuilder();` at function-body scope, which
shadowed a pre-existing nested `astBuilder` declaration later in
validateEntryPoint. MSVC's /W4 /WX treats C4456 (declaration hides previous
local) as an error, so build-windows-*-cl-x86_64-gpu failed while gcc/clang
(no -Wshadow) built clean.

Move the declaration into the loop body (leaf scope), which cannot shadow the
now-astBuilder-free function scope or the sibling nested blocks, and keeps the
call on one line. Verified shadow-free with `-Wshadow`; the fix is behavior-
preserving (getASTBuilder() is a cheap accessor).
@jkiviluoto-nv jkiviluoto-nv enabled auto-merge July 2, 2026 05:24
@jkiviluoto-nv jkiviluoto-nv added this pull request to the merge queue Jul 2, 2026
Merged via the queue into master with commit 86609ec Jul 2, 2026
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: non-breaking PRs without breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vk::binding on a resource-containing struct is honored but diagnosed as ignored vk::binding on pointer entry-point parameters is accepted but ignored

2 participants