Fix bug in matrix specialization - #12840
Conversation
A matrix type whose layout the source did not specify carries a sentinel,
SLANG_MATRIX_LAYOUT_MODE_UNKNOWN, until a target-specific pass fills in the
default. The sentinel was a plain `int`, so once it was passed as a generic
argument nothing told it apart from the row and column counts beside it:
specialize(matrixIDifferentiableWitness, Real, 4 : Int, 4 : Int, 0 : Int, witness)
specializeMatrixLayout could therefore only resolve layouts written on a matrix
type, where operand position identifies them. Specialization then substituted
the sentinel into the generic's `matrix<T, N, M, L>` and minted a fresh
unspecified-layout matrix type after the pass had already run. That reached emit
as a second, identically named _MatrixStorage struct, and an OpPtrAccessChain
mixing the two fails spirv-val.
Declare MatrixLayoutMode in the core module and use it for the layout parameter
of `matrix`. The sentinel now carries its own type wherever it travels, so one
run of the pass, before specialization, resolves it both on matrix types and in
generic arguments, and nothing downstream can reintroduce it. The pass has to
run before lowerEnumType as well, which erases MatrixLayoutMode back to `int`;
after that point the check would degenerate to "any `int` equal to 0" and would
rewrite unrelated generic arguments.
Breaking change: code that declares the layout parameter as `let L : int`, as in
`extension<T, let R : int, let C : int, let L : int> matrix<T, R, C, L>`, must
now write `let L : MatrixLayoutMode`.
GitHub Actions
Error
|
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: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughChangesThe matrix layout generic parameter now uses the public ChangesMatrix Layout Handling
Merge Risk: 🔵 Low · up to This change resolves unknown matrix layouts before specialization and adds SPIR-V regression coverage, but the public enum naming concern and validator-coverage concern remain open. These are low merge-readiness risks that should be acknowledged or addressed. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
🟡 Changes recommended
specializeMatrixLayout rewrites IR instructions via replaceUsesWith() but does not remove the replaced IRMatrixType/IRSpecialize instructions, leaving dead IR that can increase later-pass work and risks unintended processing during full-module scans.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a SPIR-V miscompile/validation failure caused by specializing generics while matrix layout is still unresolved (default float4x4 without row_major/column_major), which could previously lead to two structurally different but identically named matrix storage types reaching code generation.
Changes:
- Resolve unspecified matrix layout earlier in the IR pipeline (before specialization) and also rewrite
specialize(...)instructions that pass an unresolved layout as a generic argument. - Make matrix layout a distinct
MatrixLayoutModeenum type (instead ofint) so unresolved layout arguments remain recognizable during specialization. - Add SPIR-V regression tests to ensure specialization/layout resolution does not produce duplicate emitted types (disassembler
_0suffix).
File summaries
| File | Description |
|---|---|
| tests/spirv/matrix-layout-late-specialize.slang | New regression test covering late specialization via imported module + conformance. |
| tests/spirv/matrix-layout-late-specialize-module.slang | Helper module to force lazy cloning/specialization timing that previously triggered the bug. |
| tests/spirv/matrix-layout-dedup-specialization.slang | New regression test ensuring row_major float4x4 and float4x4 dedup under -matrix-layout-row-major. |
| source/slang/slang-ir-specialize-matrix-layout.cpp | Enhances layout specialization to also rewrite specialize args carrying Unknown layout. |
| source/slang/slang-emit.cpp | Moves specializeMatrixLayout earlier (before specialization + before enum lowering). |
| source/slang/slang-emit-spirv.cpp | Preserves the matrix-layout operand type when creating normalized debug matrix types. |
| source/slang/slang-check-expr.cpp | Uses the matrix layout’s semantic type for row/column-major modifiers (supports enum-typed layout). |
| source/slang/slang-check-decl.cpp | Same as above for layout modifiers on declarations. |
| source/slang/hlsl.meta.slang | Updates matrix-related generics to use MatrixLayoutMode for layout template parameters. |
| source/slang/diff.meta.slang | Updates matrix overload generic parameters to use MatrixLayoutMode. |
| source/slang/core.meta.slang | Introduces public MatrixLayoutMode enum and updates matrix<..., L> to use it. |
Review details
Suppressed comments (1)
source/slang/slang-ir-specialize-matrix-layout.cpp:110
- After rewriting a
specializeinstruction with resolved layout arguments, the oldIRSpecializeremains in the IR (now dead). Removing it afterreplaceUsesWith()matches the pattern used elsewhere (e.g.slang-ir-specialize.cpp) and keeps later full-module scans from doing unnecessary work.
builder.setInsertBefore(specializeInst);
auto replacement = builder.emitSpecializeInst(
specializeInst->getFullType(),
specializeInst->getBase(),
(UInt)args.getCount(),
args.getBuffer());
specializeInst->replaceUsesWith(replacement);
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| matrixType->getColumnCount(), | ||
| builder.getIntValue(builder.getIntType(), defaultLayout)); | ||
| resolvedLayout); | ||
| matrixType->replaceUsesWith(replacementMatrixType); |
There was a problem hiding this comment.
@fknfilewalker this looks like a right way to avoid a leak.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 Plus
Run ID: b45acdd7-68a1-4bac-9451-aacfbc9a1140
📒 Files selected for processing (11)
source/slang/core.meta.slangsource/slang/diff.meta.slangsource/slang/hlsl.meta.slangsource/slang/slang-check-decl.cppsource/slang/slang-check-expr.cppsource/slang/slang-emit-spirv.cppsource/slang/slang-emit.cppsource/slang/slang-ir-specialize-matrix-layout.cpptests/spirv/matrix-layout-dedup-specialization.slangtests/spirv/matrix-layout-late-specialize-module.slangtests/spirv/matrix-layout-late-specialize.slang
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
![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.
jkwak-work
left a comment
jkwak-work
left a comment
There was a problem hiding this comment.
Looks good to me, although I have two trivial comments.
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Team
Run ID: be671ede-1655-4c07-b779-dda74f8ddd2b
📒 Files selected for processing (3)
source/slang/slang-code-gen.hsource/slang/slang-emit.cppsource/slang/slang-ir-specialize-matrix-layout.cpp
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
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)
source/slang/slang-ir-specialize-matrix-layout.cpp (1)
81-83: 🎯 Functional Correctness | 🟠 MajorReuse the target-specific default matrix layout.
getMatrixLayoutMode()is not the complete lowering default.DefaultBufferElementTypeLoweringPolicyforces row-major for CPU, CUDA, and Metal, even when the option set requests column-major. This pass now resolvesUnknownbefore that policy runs, so an unspecified generic matrix can become column-major and produce a non-default storage representation on those targets. Reuse the canonical target-aware default calculation before constructingresolvedLayout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Team
Run ID: c638a443-afee-42b9-a589-1412248efef8
📒 Files selected for processing (2)
source/slang/slang-emit.cppsource/slang/slang-ir-specialize-matrix-layout.cpp
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Team
Run ID: e6564b68-b3a8-4e09-aae2-2fb416d0ae51
📒 Files selected for processing (12)
source/slang/core.meta.slangsource/slang/diff.meta.slangsource/slang/hlsl.meta.slangsource/slang/slang-check-decl.cppsource/slang/slang-check-expr.cppsource/slang/slang-code-gen.hsource/slang/slang-emit-spirv.cppsource/slang/slang-emit.cppsource/slang/slang-ir-specialize-matrix-layout.cpptests/spirv/matrix-layout-dedup-specialization.slangtests/spirv/matrix-layout-late-specialize-module.slangtests/spirv/matrix-layout-late-specialize.slang
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@nv-slang-bot it seems like the CI test failure for "SlangPy Tests" is legit. |
|
@jkwak-work Investigated — the failure is legit, and it's exactly the downstream break you'd expect from retyping the matrix layout parameter Root causeSlangPy declares its own generic extensions over the built-in Slang applies no implicit A code search across
Suggested fix (recommended)Type the layout parameter as - public extension<T, let R : int, let C : int, let L : int> matrix<T, R, C, L> : ISizedArray<vector<T, C>, R> {}
+ public extension<T, let R : int, let C : int, let L : MatrixLayoutMode> matrix<T, R, C, L> : ISizedArray<vector<T, C>, R> {}- __generic<T : IPrintable, let R : int, let C : int, let L : int>
+ __generic<T : IPrintable, let R : int, let C : int, let L : MatrixLayoutMode>
extension matrix<T, R, C, L> : IPrintable {This has to land as a companion PR in One trade-off worth a decision
If you'd rather reduce that external breakage, one option to explore is permitting an implicit I'm happy to open the companion 🤖 Generated by an automated Slang coworker — may be inaccurate. A human maintainer should verify. |
|
It appears this this PR will be a breaking change due to the changed type from @fknfilewalker , Maybe we should keep the type as |
@jkwak-work the fix only works with the enum, otherwise the information gets lost. |
|
@nv-slang-bot make a PR on slangpy repo that can pass the test with this PR. |
|
@jkwak-work — companion SlangPy PR opened (draft): shader-slang/slangpy#1135. It retypes SlangPy's two generic matrix extensions' layout param
Full details in the #1135 description. 🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify. |
I am gonna need to discuss with the team about how to proceed; regarding the backward-compatibility breaking aspect.
We may need to post an announcement.
|
This is a messy problem, due in no small part to a bunch of questionable design choices we've made over time about how to handle matrix layout in Slang. It might be impossible to implement a truly Good solution to some of this without changing the language/compiler rules. I'm trying to consider the aims of this PR as-is, though, rather than try to insist on a larger refactoring fix. At first I wasn't sure if the switch to using an explicit The
Unless I'm missing something, the simple act of making that mode into an enum should mean that the pass to apply a default matrix layout mode across an entire IR module is now basically trivial. |
|
We will proceed with the PR as it is. |
Good idea, I think with Edit: |
GitHub Actions
Error
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.
7eeaf8e to
8453258
Compare
|
FYI, I tried the following code to see if we can workaround the backward-compatibility breaking problem: But generic function overloading is not implemented by a reason that the generic types/values are not considered for the lookup. |
There was a problem hiding this comment.
Verdict: 🟡 Has issues — 2 gaps, 3 clarity nits
Changes the matrix<T,R,C,L> layout generic parameter from int to a new MatrixLayoutMode enum so an unresolved (Unknown) layout stays recognizable, and moves specializeMatrixLayout ahead of generic specialization and enum lowering so row_major float4x4 and float4x4 resolve to one type before they become distinct specialization keys. The traversal, SSA mechanics (dead specialize insts are DCE'd), pass reordering, and gating check out; the new field is zero-initialized (requiredLoweringPassSet = {}). The concerns are a fragile type-identity invariant in the detection path and test-matrix breadth.
Changes Overview
MatrixLayoutMode enum + core-module signatures (core.meta.slang, diff.meta.slang, hlsl.meta.slang)
- The fourth matrix generic parameter
LbecomesMatrixLayoutMode(values spliced fromSLANG_MATRIX_LAYOUT_*) instead ofint, across all matrix extensions/generics. Markedpr: breaking change.
Layout-type propagation at construction sites (slang-check-decl.cpp, slang-check-expr.cpp, slang-emit-spirv.cpp)
- Build the layout
IntVal/IRIntLitwith the matrix's actual layout type (getLayout()->getType()/getFullType()) rather than a bare int type, so the operand carriesMatrixLayoutMode.
Pass rewrite + reordering (slang-ir-specialize-matrix-layout.cpp, slang-emit.cpp, slang-code-gen.h)
specializeMatrixLayoutnow collects both unknown-layout matrix types andspecializeinsts that passUnknownas a generic argument, resolving both to the target default; runs before specialization/enum-lowering, gated by a newunresolvedMatrixLayoutflag incalcRequiredLoweringPassSet.
Tests (tests/spirv/matrix-layout-dedup-specialization.slang, matrix-layout-late-specialize.slang, matrix-layout-late-specialize-module.slang)
- Cover the direct-matrix-type dedup path and the late/generic-specialization path; both assert a single storage/struct type via positive-anchor +
-NOT _0.
Findings (5 total)
| Severity | Location | Finding |
|---|---|---|
| 🟡 Gap | slang-ir-specialize-matrix-layout.cpp:42 |
Unknown-layout detection keys on IR type identity; comment says "silently skipped" but debug asserts, and a plain-int Unknown literal (older serialized module) is silently skipped in release, reintroducing the duplicate-storage bug. |
| 🟡 Gap | tests/spirv/matrix-layout-dedup-specialization.slang:1 |
Only row-major default and SPIR-V target tested; column-major default and non-SPIR-V backends (both affected) uncovered. |
| 🟡 Nit | core.meta.slang:2303 |
New MatrixLayoutMode enum duplicates adjacent k*MatrixLayout int constants with no note on the relationship. |
| 🟡 Nit | slang-emit.cpp:650 |
as<IRMatrixType>(inst) inside case kIROp_MatrixType guards an impossible shape; prefer cast<>. |
| 🟡 Nit | slang-ir-specialize-matrix-layout.cpp:53 |
visitSpecializeInsts ordering precondition documented but not asserted. |
reviewed: 8453258 · diff sha256 8bfe93b51c2c
| auto layout = matrixType->getLayout(); | ||
| if (!matrixLayoutModeType) | ||
| matrixLayoutModeType = layout->getFullType(); | ||
| SLANG_ASSERT(layout->getFullType() == matrixLayoutModeType); |
There was a problem hiding this comment.
🟡 Gap: unknown-layout detection keys on IR type identity, with a fail-silent release path
isUnknownMatrixLayout recognizes an unknown layout only when the literal's type is identical to the one MatrixLayoutMode instance captured from the first matrix seen:
auto lit = as<IRIntLit>(inst);
if (!lit || lit->getFullType() != matrixLayoutModeType) // line 17
return false;
return lit->getValue() == SLANG_MATRIX_LAYOUT_MODE_UNKNOWN;and visitMatrixTypes enforces that all matrix layout operands share that one type:
SLANG_ASSERT(layout->getFullType() == matrixLayoutModeType); // line 42Two problems:
-
The comment contradicts the code. The preceding comment says a mismatch "would be silently skipped," but
SLANG_ASSERTfires in debug — it is silent only in release (where the assert is compiled out and thegetFullType()check inisUnknownMatrixLayoutdrops the literal). Please make the comment describe the actual dual behavior. -
A plain-
int-typed Unknown layout literal (value 0) would be silently skipped in release, leaving that matrix unresolved and lowering it to a second, identically-named storage type — exactly the duplicate-_MatrixStoragebug this PR fixes. Before this PR the layout operand was plainint, andk_maxSupportedModuleVersion/k_minSupportedModuleVersion(slang-ir.h:2260-2261) were not bumped, so a.slang-moduleserialized by an older compiler can still deserialize with anint-typed layout literal. Linking such a module alongside a freshMatrixLayoutMode-typed compile puts both spellings of Unknown in one module.
Note the type check is intentional — it distinguishes an enum-typed Unknown (value 0) from an unrelated int generic argument of value 0 (e.g. Bar<0>), so simply ignoring the type is not the fix. Please either (a) confirm that a plain-int Unknown matrix-layout literal can never reach this pass (state the invariant and make it a SLANG_RELEASE_ASSERT rather than a debug-only assert + silent release skip), or (b) normalize the layout literal's type on deserialization/linking so the identity check holds.
Example: compile A.slang (uses float4x4) to a module with a pre-this-PR compiler → its layout literal is int-typed. import "A" from a new-compiler shader that also uses float4x4. In release, A's matrix stays Unknown and emits a duplicate storage struct; in debug the assert aborts.
| @@ -0,0 +1,25 @@ | |||
| //TEST:SIMPLE(filecheck=CHECK): -target spirv-asm -entry main -stage compute -matrix-layout-row-major | |||
There was a problem hiding this comment.
🟡 Gap: only the row-major default and the SPIR-V target are exercised
Both new tests pin -matrix-layout-row-major:
//TEST:SIMPLE(filecheck=CHECK): -target spirv-asm -entry main -stage compute -matrix-layout-row-major
The rewritten pass resolves every unknown layout to resolvedLayout = builder.getIntValue(collector.matrixLayoutModeType, defaultLayout) where defaultLayout = target->getOptionSet().getMatrixLayoutMode(). Two behaviors that this PR changes are never covered:
- Column-major default. Column-major is the more common Vulkan/SPIR-V default, and it flows a different value through both
resolvedLayoutand the new specialize-inst rebuild. A bug that hard-codes/mishandles row-major would still pass CI. - Non-SPIR-V targets. The dedup and the pass-ordering move happen in
linkAndOptimizeIRfor all backends (aWrapper<row_major float4x4>/Wrapper<float4x4>pair specializes once for HLSL/GLSL/CPU too), but all three tests are SPIR-V-only.
Suggestion: add a -matrix-layout-column-major variant of the dedup test asserting the two spellings still collapse to one struct (same positive-anchor + -NOT _0 pattern), and consider one non-SPIR-V (e.g. -target hlsl or CPU compute) variant so the target-independent fix is exercised outside SPIR-V.
| /// @remarks `Unknown` takes the order selected for the target. This is an enum rather than an | ||
| /// `int` so an unresolved layout stays recognizable when passed as a generic argument. | ||
| /// @category math_types Math types | ||
| enum MatrixLayoutMode : int |
There was a problem hiding this comment.
🟡 Gap: new MatrixLayoutMode enum duplicates the adjacent k*MatrixLayout constants without noting the relationship
MatrixLayoutMode.RowMajor/.ColumnMajor now carry the same values as kRowMajorMatrixLayout/kColumnMajorMatrixLayout immediately above (lines 2295-2296). Two spellings of the same values now coexist with no comment on which callers should use, or whether the k* constants are kept only for source compatibility.
Suggestion: add a one-line note stating the relationship (and whether the k* constants are now superseded/retained for compatibility) so a future maintainer doesn't have to guess whether they are redundant.
| case kIROp_MatrixType: | ||
| // An `Unknown` layout needs the pass. So does `Unknown` passed as a generic argument, | ||
| // which this scan cannot recognize, so a generic (non-literal) layout requests it too. | ||
| if (auto matrixType = as<IRMatrixType>(inst)) |
There was a problem hiding this comment.
🟡 Nit: as<IRMatrixType> inside case kIROp_MatrixType guards an impossible shape
case kIROp_MatrixType:
...
if (auto matrixType = as<IRMatrixType>(inst))Within case kIROp_MatrixType, inst is already known to be an IRMatrixType, so this as<> null-check can never fail — it reads as defensive handling of an impossible shape. Per CLAUDE.md ("fail loudly on out-of-contract input / don't guard impossible shapes"), an unconditional cast<IRMatrixType>(inst) states the invariant more clearly than an if whose false branch is dead.
|
|
||
| // Collects the `specialize` insts that pass `Unknown` as a generic argument. | ||
| // Needs `matrixLayoutModeType`, so call after `visitMatrixTypes`. | ||
| void visitSpecializeInsts(IRInst* parent) |
There was a problem hiding this comment.
🟡 Nit: visitSpecializeInsts' ordering precondition is documented but not enforced
// Needs `matrixLayoutModeType`, so call after `visitMatrixTypes`.
void visitSpecializeInsts(IRInst* parent)If matrixLayoutModeType is still null (i.e. visitMatrixTypes hasn't run), every isUnknownMatrixLayout call returns false and this walk silently collects nothing — a reordering yields a silent miss rather than a failure. The single caller orders the two correctly and early-returns on a null type, so this is not a live bug, but consider a SLANG_ASSERT(matrixLayoutModeType) at entry so the internal contract can't be violated silently.
GitHub Actions
Waiting
When a shader writes
float4x4without sayingrow_majororcolumn_major, the layout stays unresolved until a later pass fills in the target's default. If that unresolved layout is handed to a generic first, specialization bakes it in and builds a matrix type the pass has already run past, so it never gets fixed.The result is two versions of what should be one type reaching code generation: same name, different layout. Mixing them produces SPIR-V that fails validation.
This makes the layout get resolved before specialization can copy it around, so only one version is ever created.