Fix default value lowering for fields of associated type - #12435
Fix default value lowering for fields of associated type#12435fknfilewalker wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughChangesThe compiler now processes matrix layouts after specialization and constructs specialization-dependent types explicitly. New SPIR-V tests cover associated-type default initialization and late matrix-layout specialization. Specialization and Default Construction
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (4 skipped: 3 unsupported, 1 too large.) 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a5cc3ff8-f95c-4648-a22e-f4d8d5492ce8
📒 Files selected for processing (3)
source/slang/slang-emit.cppsource/slang/slang-lower-to-ir.cpptests/spirv/assoc-type-default-init.slang
There was a problem hiding this comment.
Pull request overview
This PR fixes two sources of invalid SPIR-V produced when types are only fully resolved during/after specialization: (1) associated-type default values incorrectly lowering to a zero-operand makeStruct that later becomes an invalid scalar OpConstantComposite, and (2) matrix types with unknown/default layout being instantiated after the matrix-layout specialization pass, causing duplicate matrix storage structs and invalid pointer access chains.
Changes:
- Adjust default-value lowering so
AssocTypeDecldefaults lower toDefaultConstruct(instead of member-wisemakeStruct), allowing later specialization/peepholes to materialize a valid concrete default. - Move
specializeMatrixLayoutto run afterfinalizeSpecializationso it also covers matrix types created from lazily imported generic bodies during specialization. - Add a SPIR-V regression test covering the associated-type default initialization case (ensuring no scalar-typed
OpConstantCompositeis emitted).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/spirv/assoc-type-default-init.slang | New SPIR-V test guarding against invalid scalar OpConstantComposite from associated-type default initialization after specialization. |
| source/slang/slang-lower-to-ir.cpp | Treat AssocTypeDecl default values like interfaces: lower to emitDefaultConstruct to avoid zero-operand makeStruct on unresolved associated types. |
| source/slang/slang-emit.cpp | Reorder passes so specializeMatrixLayout runs after finalizeSpecialization, covering lazily imported generics that instantiate matrices later in the pipeline. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/spirv/assoc-type-default-init.slang (1)
1-1: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSet SPIR-V validation configuration explicitly.
CI runs
SLANG_RUN_SPIRV_VALIDATION=1, but the test file itself does not set it whileFileCheckchecks onlyOpEntryPoint. Add a wrapper/test invocation with this variable or use the harness to make this regression test self-contained.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3dd661e5-a22d-42ba-a54a-c3e1cc7f5a7a
📒 Files selected for processing (1)
tests/spirv/assoc-type-default-init.slang
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c8c2b98b-caca-4066-ae5c-2923091a42e1
📒 Files selected for processing (2)
tests/spirv/matrix-layout-late-specialize-module.slangtests/spirv/matrix-layout-late-specialize.slang
c4428b8 to
bc21376
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GitHub Actions
Inactive
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Failure
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GitHub Actions
Inactive
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GitHub Actions
Inactive
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GitHub Actions
Error
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GitHub Actions
Error
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
6260c69 to
ddbbc85
Compare
GitHub Actions
Error
GitHub Actions
Error
fknfilewalker
changed the title
GitHub Actions
Error
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jkwak-work
removed
Office-Tess
The default value for a field of associated type used to lower as a zero-operand makeStruct; once specialization resolved the type to a scalar, that emitted an OpConstantComposite with a scalar result type, which spirv-val rejects. The lowering fix landed upstream in shader-slang#12712 (getDefaultVal now only takes the member-wise makeStruct path for a concrete field-owning aggregate, and otherwise falls through to emitDefaultConstruct), but that PR added no test. This keeps the regression test for it.
Generic bodies pulled in during specialization can mention matrix with the defaulted (unknown) layout, creating matrix types after the pass ran. The layout-only duplicates lower to distinct but identically named _MatrixStorage structs, and an OpPtrAccessChain mixing them fails spirv-val because its result type must be the base's exact type id. Run the pass once specialization is done so those types are covered as well.
CHECK-NOT is bounded by the next positive match, and OpEntryPoint precedes the constants section, so the negative check scanned a region where OpConstantComposite cannot appear. Move it after the OpEntryPoint match; verified the reordered directives fail against the unfixed compiler's output.
The scenario requires a -conformance naming a specialized generic from an imported module: only then is the generic body - carrying matrix<T, R, C> with the defaulted (unknown) layout - cloned during specializeModule, after the early specializeMatrixLayout run. Verified the test fails against a compiler without the pass move (duplicate _MatrixStorage_float4x4natural_0 in the output, all occurrences after the OpEntryPoint anchor) and passes with it.
- Note why deferring specializeMatrixLayout past specialization is safe. - Strengthen tests: cover vector resolution of the associated type and add positive CHECK anchors so the CHECK-NOTs cannot pass vacuously. The default-value branch fold is dropped: upstream shader-slang#12712 now gates the member-wise makeStruct path on isConcreteFieldOwningAggregate, so AssocTypeDecl / ThisTypeDecl / GlobalGenericParamDecl already fall through to emitDefaultConstruct and an explicit branch for them would be redundant.
Leaves this branch with the associated-type default-value fix only. The specializeMatrixLayout change and its tests move to matrix-layout-mode-enum, where the layout is given its own type so an unresolved layout stays recognizable after it is passed as a generic argument.
f2c00cb to
db68698
Compare
GitHub Actions
Waiting
|
closed because fixed by #12708 |
There was a problem hiding this comment.
Verdict: 🟡 Minor — 0 bugs, 1 (low) gap, plus 2 clarity notes on the test file.
This PR's net diff against master adds exactly one regression test, tests/spirv/assoc-type-default-init.slang; the underlying getDefaultVal fix (the isConcreteFieldOwningAggregate guard + emitDefaultConstruct fallthrough at source/slang/slang-lower-to-ir.cpp:6736,6764) already landed in master, so this review covers the test only. The test correctly reproduces the bug — a zero-constituent OpConstantComposite from a zero-operand makeStruct on an associated-type field default — and its CHECK-NOT ...{{$}} assertions would catch a naive reintroduction. Findings are about coverage breadth and maintenance robustness, not correctness of the guard.
Changes Overview
Regression test for associated-type field default init (tests/spirv/assoc-type-default-init.slang)
- Adds a
SIMPLE/-target spirvFileCheck test: a genericstruct P<Real:IReal> : IDifferentiablewith an associated-type field, instantiated atfloatandfloat3.CHECK-NOTlines assert no scalar/vector-result zero-constituentOpConstantCompositeis emitted; a positiveOpFunctionCallline bounds the range. In CI (SLANG_RUN_SPIRV_VALIDATION=1) the pre-fix compiler additionally fails spirv-val, so the positiveCHECK: OpEntryPointwould also fail.
Findings (3 total)
| Severity | Location | Finding |
|---|---|---|
| 🟡 Gap | tests/spirv/assoc-type-default-init.slang:41 |
Sibling ThisType/global-generic default-init paths sharing the same emitDefaultConstruct fallthrough are untested |
| 🔵 Clarity | tests/spirv/assoc-type-default-init.slang:49 |
Positive CHECK couples the test to a synthesized __init name and one lowering strategy; range/type-coupling of the CHECK-NOTs is implicit |
| 🔵 Clarity | tests/spirv/assoc-type-default-init.slang:25 |
[noinline] is load-bearing for the repro but its purpose is unstated |
reviewed: db68698 · diff sha256 6804e51ad131
| Real area; | ||
| } | ||
|
|
||
| [noinline] |
There was a problem hiding this comment.
🔵 Clarity: [noinline] looks load-bearing but its purpose is unstated
[noinline] appears essential to reproducing the bug: if use were inlined, the P<Real>.Differential(v) construction and its associated-type-field default could be constant-folded away before the malformed OpConstantComposite is emitted, so the test would pass even against the pre-fix (buggy) compiler. Because the whole file is a regression guard, a future maintainer cleaning up an "unnecessary" attribute could silently defeat it.
Suggestion: Add a one-line comment stating that inlining must be prevented so the associated-type default survives to codegen. (If [noinline] is not actually required to reproduce, dropping it would simplify the repro instead.)
| [numthreads(1, 1, 1)] | ||
| void main(uint3 tid: SV_DispatchThreadID) | ||
| { | ||
| outBuf[0] = get<float>(float3(tid.x, 1, 2)) + get<float3>(float3(tid.x, 3, 4)); |
There was a problem hiding this comment.
🟡 Gap: sibling associated-type-like default-init paths (ThisType, global generic param) are not covered
The fix this test guards routes AssocTypeDecl to emitDefaultConstruct via the fallthrough at source/slang/slang-lower-to-ir.cpp:6764. ThisTypeDecl and GlobalGenericParamDecl also derive from AggTypeDecl (source/slang/slang-ast-decl.h), are not concrete-field-owning, and rely on the same fallthrough — yet nothing exercises a default value for a field whose type is a This type or a global generic parameter. The InterfaceDecl sibling is separately covered by tools/slang-unit-test/unit-test-spirv-interface-default-init-validation.cpp; these two are not.
Criticality is low (they share the fallback with the tested AssocType case), but a future refactor of that branch could regress them without any test failing.
Suggestion: Optionally add a this-typed and/or global-generic-typed field default-init case (here, or in the existing interface-default-init validation test) so the whole AggTypeDecl-fallthrough family is guarded, not just the AssocType instance.
| // CHECK: OpEntryPoint | ||
| // CHECK-NOT: OpConstantComposite %float{{$}} | ||
| // CHECK-NOT: OpConstantComposite %v3float{{$}} | ||
| // CHECK: OpFunctionCall %P_Differential %P_Differential__init {{.*}} %float_0 |
There was a problem hiding this comment.
🔵 Clarity: the positive CHECK couples the test to a synthesized name and one lowering strategy
The real regression guard is the two CHECK-NOT: OpConstantComposite ...{{$}} lines above — the {{$}} end-anchor precisely catches a zero-constituent composite (a well-formed default has constituents after the result type). This final positive CHECK additionally pins the output to a specific synthesized name (%P_Differential__init) and to the current lowering (default → non-inlined init call passing %float_0), and only anchors the float instantiation. A later still-correct change that folds the default into a valid non-scalar constant, renames the synthesized __init, or inlines the constructor would break this line even though the guarded bug has not regressed.
Two smaller clarity points on the same block:
- The comment ("the positive check anchors the range") is doing real work: the
CHECK-NOTs only guard the region between theOpEntryPointandOpFunctionCallmatches. That correctly covers the SPIR-V types/constants section, but a reader has to know constants sit afterOpEntryPointand before function bodies to see the anchors bracket the right region — worth a one-line note. - The
%float/%v3floatCHECK-NOTs are tied to the two instantiations inmain; if those types change, the guards silently stop matching. A brief note tying them to theget<float>/get<float3>calls would keep that coupling visible.
Suggestion: Document that this positive line exists only as the closing range anchor for the CHECK-NOTs, or use a looser anchor (e.g. OpFunction) that bounds the same range with less coupling to naming/inlining.
getDefaultValtreated an associated type like a struct and emitted a member-wisemakeStruct. An associated type has no fields, so themakeStructgot zero operands, and once specialization resolved the type to a scalar (e.g.Real.Differential->float) the emitter produced anOpConstantComposite %floatwith no constituents, which spirv-val rejects.Since the resolved type may not be a struct at all, emit
DefaultConstructinstead; the existing peephole folds it to a concrete zero once the type is known. The same applies to the other decls whose concrete type is unknown before specialization (InterfaceDecl,ThisTypeDecl,GlobalGenericParamDecl).