[SM6.10] LinAlg Validation: MatrixOuterProduct - #8808
[SM6.10] LinAlg Validation: MatrixOuterProduct#8808Ashley Coleman (V-FEXrt) wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Implements Shader Model 6.10 validation rules for MatrixOuterProduct.
Changes:
- Validates matrix scope, use, dimensions, and element types.
- Adds validation diagnostics and component-type helpers.
- Adds DXIL validation tests and corrects nominal code-generation expectations.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
utils/hct/hctdb.py |
Adds and generalizes validation rules. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-outerproduct.ll |
Tests valid and invalid outer products. |
tools/clang/test/CodeGenDXIL/hlsl/linalg/builtins/matrixouterproduct/nominal.hlsl |
Updates nominal matrix attributes. |
lib/DxilValidation/DxilValidationUtils.h |
Declares component-type helpers. |
lib/DxilValidation/DxilValidationUtils.cpp |
Implements component-type helpers. |
lib/DxilValidation/DxilValidation.cpp |
Implements outer-product validation. |
docs/DXIL.rst |
Documents the new diagnostics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Alex Sepkowski (alsepkow)
left a comment
Alex Sepkowski (alsepkow)
left a comment
There was a problem hiding this comment.
LGTUS: A few non blocking comments
Finn Plummer (@inbelic) Deric C. (@Icohedron)
| VectorType *BVecTy = cast<VectorType>(Op.get_vectorB()->getType()); | ||
| std::optional<LinAlgTargetType> RetMat = | ||
| GetCheckedLATT(CI->getType(), ValCtx); | ||
| if (!RetMat) |
There was a problem hiding this comment.
Would this be better as an assert?
There was a problem hiding this comment.
It should not be an assert! GetCheckedLATT prints an error for the user saying its wrong/failing the shader validation. If we assert here then the validator falls over instead of cleaning exiting with failures
|
|
||
| define void @main() { | ||
| ; okay | ||
| %1 = call %dx.types.LinAlgMatrixC8M8N8U2S0 @dx.op.linAlgMatrixOuterProduct.mC8M8N8U2S0.v8f16.v8f16(i32 -2147483619, <8 x half> <half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00>, <8 x half> <half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000>) ; LinAlgMatrixOuterProduct(vectorA,vectorB) |
There was a problem hiding this comment.
do we need a check here?
There was a problem hiding this comment.
same comment as the other PR, yes, but I'd rather do it in a batch PR once everything merges
| %1 = call %dx.types.LinAlgMatrixC8M8N8U2S0 @dx.op.linAlgMatrixOuterProduct.mC8M8N8U2S0.v8f16.v8f16(i32 -2147483619, <8 x half> <half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00>, <8 x half> <half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000>) ; LinAlgMatrixOuterProduct(vectorA,vectorB) | ||
|
|
||
| ; okay | ||
| %2 = call %dx.types.LinAlgMatrixC21M8N8U2S0 @dx.op.linAlgMatrixOuterProduct.mC21M8N8U2S0.v8f16.v8f16(i32 -2147483619, <8 x half> <half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00>, <8 x half> <half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00>) ; LinAlgMatrixOuterProduct(vectorA,vectorB) |
There was a problem hiding this comment.
check here?
There was a problem hiding this comment.
In cases that aren't the first okay I don't think we can add a check because we already have the chain of CHECK-NEXTs ensuring no "extra" stuff gets printed
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-outerproduct.ll:15
- These three cases are labeled as valid, but the first positive
CHECKappears only at the first invalid call. FileCheck ignores unmatched output before that match, so an erroneous diagnostic for any of these calls would not fail the test. Add a leadingCHECK-NOTassertion (or move the valid cases to a%dxvsuccess test) so their acceptance is actually covered.
This issue also appears on line 31 of the same file.
; okay
%1 = call %dx.types.LinAlgMatrixC8M8N8U2S0 @dx.op.linAlgMatrixOuterProduct.mC8M8N8U2S0.v8f16.v8f16(i32 -2147483619, <8 x half> <half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00>, <8 x half> <half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000>) ; LinAlgMatrixOuterProduct(vectorA,vectorB)
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-outerproduct.ll:33
- This case changes both M and N, while the next case isolates only N. The test would therefore still pass if the new
RetMat->M != Mvalidation were removed. Make this a16x8result (updating its declaration and metadata), or add a separate M-only mismatch case, so both required dimension checks are independently covered.
; CHECK-NEXT: Function: main: error: Return matrix dimension '16x16' must match derived matrix dimension '8x8'.
; CHECK-NEXT: note: at {{.*}} @dx.op.linAlgMatrixOuterProduct.mC8M16N16U2S0.v8f16.v8f16
%6 = call %dx.types.LinAlgMatrixC8M16N16U2S0 @dx.op.linAlgMatrixOuterProduct.mC8M16N16U2S0.v8f16.v8f16(i32 -2147483619, <8 x half> <half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00, half 0xH3C00>, <8 x half> <half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000, half 0xH4000>) ; LinAlgMatrixOuterProduct(vectorA,vectorB)
Fixes #8508
Implements MatrixOuterProduct validation rules