[SM6.10] LinAlg Validation: Convert - #8837
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Pull request overview
Implements Shader Model 6.10 validation rules for LinAlg Convert operations.
Changes:
- Validates interpretation constants, vector element types, and output dimensions.
- Adds validation diagnostics and DXIL documentation.
- Adds and updates validation/code-generation tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
lib/DxilValidation/DxilValidation.cpp |
Implements Convert validation. |
lib/DxilValidation/DxilValidationUtils.cpp |
Identifies native component types. |
lib/DxilValidation/DxilValidationUtils.h |
Declares the native-type helper. |
utils/hct/hctdb.py |
Defines new validation messages. |
docs/DXIL.rst |
Documents the new rules. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-convert.ll |
Tests validation failures. |
tools/clang/test/CodeGenDXIL/hlsl/linalg/builtins/convert/nominal.hlsl |
Updates nominal inputs to valid interpretations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Damyan Pepper (damyanp)
left a comment
There was a problem hiding this comment.
LGTM, although some of the copilot comments look like they need addressing.
3fdd8f9 to
2832abc
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The vector-length diagnostic incorrectly references a matrix, and the user-visible validation change needs preview release-note coverage.
Review details
Suppressed comments (2)
lib/DxilValidation/DxilValidation.cpp:1620
- This reuses the matrix-dimension diagnostic for
LinAlgConvert, so an invalid conversion reports that its result must match an “input matrix M dimension” even though this operation has no matrix (as the new test demonstrates). Please add a Convert-specific vector-length rule, e.g. one that says the output vector size must match the expected converted size, and update the generated documentation/test expectation accordingly.
CI, ValidationRule::InstrLinAlgMatrixDimVectorMismatch,
{"Return", std::to_string(RetVecTy->getNumElements()),
std::to_string(ExpectedOutVecSize)});
lib/DxilValidation/DxilValidation.cpp:1549
- This adds user-visible validation behavior for experimental Shader Model 6.10, but the PR does not add an entry under
docs/ReleaseNotes.md→Upcoming Preview Release. Please add a release note for the new LinAlg Convert validation, or point to the related stacked PR that will provide the shared release-note coverage.
ValidateLinAlgOpParameters(CI, ValCtx);
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The diagnostics contain typos, and the user-visible SM6.10 validation behavior needs release-note coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
lib/DxilValidation/DxilValidation.cpp:1551
- This adds user-visible validation behavior for experimental Shader Model 6.10, but the PR does not add an entry under
docs/ReleaseNotes.md→ “Upcoming Preview Release.” Please add an entry for the Convert validation rules, or point to the shared release-note coverage in the stacked effort.
ValidateLinAlgOpParameters(CI, ValCtx);
- Files reviewed: 7/7 changed files
- Comments generated: 4
- Review effort level: Balanced
| INSTR.LINALGMATRIXUNSIGNEDFLOATTYPENOTALLOWED Float-like type '%0' must be signed | ||
| INSTR.LINALGMATRIXUSEMISMATCH %0 matrix use '%1' does not match expected use %2. | ||
| INSTR.LINALGMATRIXUSEMISMATCH2 %0 matrix use '%1' does not match expected use %2 or %3. | ||
| INSTR.LINALGMATRIXVECELEMCOUNTMISMATCH Return vector size '%0' must must size '%1' derived from input vector size and type. |
| "OutputInterpretation"); | ||
|
|
||
| // The remaining validations only give reasonable errors when assuming valid | ||
| // ComponentTypes. Stop early to minimze noise/avoid being unhelpful |
| ; CHECK-NEXT: note: at {{.*}} @dx.op.linAlgConvert.v32i32.v8f32 | ||
| %12 = call <32 x i32> @dx.op.linAlgConvert.v32i32.v8f32(i32 -2147483618, <8 x float> <float 9.000000e+00, float 8.000000e+00, float 7.000000e+00, float 6.000000e+00, float 5.000000e+00, float 4.000000e+00, float 3.000000e+00, float 2.000000e+00>, i32 21, i32 4) ; LinAlgConvert(inputVector,inputInterpretation,outputInterpretation) | ||
|
|
||
| ; CHECK-NEXT: Function: main: error: Return vector size '32' must must size '2' derived from input vector size and type. |
| ) | ||
| self.add_valrule( | ||
| "Instr.LinAlgMatrixVecElemCountMismatch", | ||
| "Return vector size '%0' must must size '%1' derived from input vector size and type.", |
Fixes #8509 Implements LinAlg Convert validation rules
999b6e5 to
93eaf10
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Release-note coverage is missing, and several newly added messages contain typos.
Review details
Suppressed comments (5)
utils/hct/hctdb.py:8756
- The diagnostic is grammatically malformed: “must must size” should be “must match size.” Please update the generated documentation and test expectation as well.
"Return vector size '%0' must must size '%1' derived from input vector size and type.",
docs/DXIL.rst:3234
- Keep this generated diagnostic text consistent with the corrected validation rule: “must must size” should be “must match size.”
INSTR.LINALGMATRIXVECELEMCOUNTMISMATCH Return vector size '%0' must must size '%1' derived from input vector size and type.
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-convert.ll:50
- The expected diagnostic repeats the malformed “must must size” wording. Update it to match the corrected validation message.
; CHECK-NEXT: Function: main: error: Return vector size '32' must must size '2' derived from input vector size and type.
lib/DxilValidation/DxilValidation.cpp:1581
- Correct the typo “minimze” to “minimize.”
// ComponentTypes. Stop early to minimze noise/avoid being unhelpful
lib/DxilValidation/DxilValidation.cpp:1552
- This introduces user-visible validation behavior for experimental Shader Model 6.10, so please add an entry under
### Upcoming Preview Releaseindocs/ReleaseNotes.md, or point to the shared release-note coverage planned for this stacked effort.
DxilInst_LinAlgConvert Op(CI);
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Joshua Batista (bob80905)
left a comment
There was a problem hiding this comment.
+1 on the Copilot comment on the double-must typo
| return; | ||
| DXIL::ComponentType InputInterp = | ||
| static_cast<DXIL::ComponentType>(*InputInterpV); | ||
| IsComponentTypeValid &= ValidateLinAlgComponentType(CI, InputInterp, ValCtx, |
There was a problem hiding this comment.
A nit: if component type is invalid at this point, we will continue and attempt to validate the constant-ness of the output interp value.
One could argue we should return upon the first validation error.
Is this behavior intentional?
i.e., does a
if (!IsComponentTypeValid)
return;
belong here?
| ) | ||
| self.add_valrule( | ||
| "Instr.LinAlgMatrixVectorTypeMustMatch", | ||
| "%0 vector element type '%1' must match %2 matrix element type '%3'.", |
There was a problem hiding this comment.
Surprised this diagnostic doesn't already exist / can't be merged with another pre-existing diagnostic.
Can't see anything this can be merged with.
Fixes #8509
Implements LinAlg Convert validation rules
Stack created with GitHub Stacks CLI • Give Feedback 💬