[SM6.10] LinAlg Validation: MatrixLength - #8787
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds DXIL validation for dx.op.linAlgMatrixLength to enforce allowed matrix scopes, and updates the corresponding lit test to assert the new diagnostic.
Changes:
- Enforce that
LinAlgMatrixLengthinput matrices are scoped toWaveorThreadGroup. - Extend the LinAlgMatrix non-thread-ops test to cover the new validation error and intrinsic declaration.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-non-thread-ops.ll | Adds a LinAlgMatrixLength call and CHECK lines validating the new scope-mismatch diagnostic. |
| lib/DxilValidation/DxilValidation.cpp | Implements scope validation for dx.op.linAlgMatrixLength and emits a format error when scope is not Wave/ThreadGroup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ValidateLinAlgOpParameters(CI, ValCtx); | ||
| DxilInst_LinAlgMatrixLength Op(CI); | ||
| std::optional<LinAlgTargetType> Mat = | ||
| GetCheckedLATT(Op.get_matrix()->getType(), ValCtx); | ||
| if (!Mat) | ||
| return; |
There was a problem hiding this comment.
This is already ensured by the type system and an extremely common pattern. Generally we should be able to assume well-formed IR from the frontend.
There was a problem hiding this comment.
I'm not sure I agree with "we should be able to assume well-formed IR from the frontend." - the whole point of the validator is to ensure that code from other frontends is valid.
At the same time, I don't think it matters so much about hitting an assert in debug builds, as long as a non-assert enabled build rejects the invalid dxil.
There was a problem hiding this comment.
Well-formed and valid aren't the same thing :)
A proper frontend will not generate IR that calls a function with the wrong types for example. If we can't assume that the frontend generated a call to foo(i32, float) with parameters of i32, float then pretty much all bets are off and the backend needs to have essentially another copy of the frontend in it. (or at least the type checker)
Separately, if we say it's only valid to call foo with even integers then that's where we can't necessarily trust the frontend to not generate IR with odd integers so we validate.
In this specific case, we only have overloads for linAlgMatrixLength with a matrix in param slot 1, if the frontend ever generates anything different then the type system is very broken and the IR is not well-formed
There was a problem hiding this comment.
Isn't well-formedness validated elsewhere already before we get to this point?
Anyway, to be clear, I don't think this impacts this PR, but alarm bells go off if I hear anything suggesting that the validator should lean on the frontend, because the frontend isn't always there and is not something that's in the control of the validator.
There was a problem hiding this comment.
LLVM IR well-formedness is already validated elsewhere yeah but DXIL is a bit weird in this regard (in fact DXIL makes a lot of this weird since other folks generate it directly and its not really an IR anymore).
As long as the function is declared correctly then we should expect a type error to already be generated if the wrong type is passed in, but I think copilot is commenting here because nothing stops someone from declaring an illegal overload of matrixlength that doesn't have a matrix as arg 1. That would be well-formed LLVM IR that passes the IR checker but not well-formed DXIL since its an illegal overload.
Out of curiosity I'll hand build both of those rq and report back.
I suppose the question we need to answer more generally is: what is our responsibility to users who forgo the safety checks of the frontend and generate DXIL directly? Should we gracefully fail for all generatable DXIL thats well-formed enough to pass llvmir checks yet not well-formed DXIL? There is also certainly a gray area here on whether we should call illegal type overload for DXIL operations as "invalid" or "not well-formed" and I suppose I'm slowly talking myself into the first
aside: I do have to admit I found myself a bit snippy with copilot for calling out a pretty mundane thing as a "HIGH" priority issue after having seen the pattern at least 10 other times without saying anything about it. It reads a bit as trying to invent a comment when it had nothing else to say
There was a problem hiding this comment.
Out of curiosity I'll hand build both of those rq and report back.
Calling with correct declaration but wrong parameter type errors as expected
shader: invalid forward reference to function 'dx.op.linAlgMatrixLength.mC8M4N4U2S2' with wrong type!
%2 = call i32 @dx.op.linAlgMatrixLength.mC8M4N4U2S2(i32 -2147483632, i32 1) ; LinAlgMatrixLength(matrix)
Declaring a wrong overload for the operation (but calling it correctly) errors with an invalid overload error
Function: main: error: 'dx.op.linAlgMatrixLength.mC8M4N4U2S2' is not a DXILOpFuncition for DXILOpcode 'LinAlgMatrixLength'.
note: at '%2 = call i32 @dx.op.linAlgMatrixLength.mC8M4N4U2S2(i32 -2147483632, i32 1)' in block '#0' of function 'main'.
Function: main: error: DXIL intrinsic overload must be valid.
note: at '%2 = call i32 @dx.op.linAlgMatrixLength.mC8M4N4U2S2(i32 -2147483632, i32 1)' in block '#0' of function 'main'.
Function: dx.op.linAlgMatrixLength.i32: error: External function 'dx.op.linAlgMatrixLength.i32' is unused.
So we properly catch both cases already and copilots comment was actually just superfluous. That said, my reason for rejecting the comment was also incorrect so I guess its a learning moment:)
Fixes #8786
Add MatrixLength Validation rule