[SM6.10] LinAlg Validation: MatrixStoreToMemory - #8834
Conversation
Fixes #8498 Implements LinAlg MatrixStoreToMemory validation rules
There was a problem hiding this comment.
Pull request overview
Implements Shader Model 6.10 validation for LinAlgMatrixStoreToMemory.
Changes:
- Validates matrix scope, groupshared type/capacity, offset, and stride.
- Adds validation rules, diagnostics, and type utilities.
- Adds validation coverage and updates CodeGen fixtures.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
utils/hct/hctdb.py |
Defines new validation diagnostics. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-matrixstoretomemory.ll |
Tests store validation rules. |
tools/clang/test/CodeGenDXIL/hlsl/linalg/builtins/matrixstoretomemory/vector-array.hlsl |
Updates vector-array fixture. |
tools/clang/test/CodeGenDXIL/hlsl/linalg/builtins/matrixstoretomemory/nominal.hlsl |
Updates nominal fixture. |
lib/DxilValidation/DxilValidationUtils.h |
Declares native-type comparison helper. |
lib/DxilValidation/DxilValidationUtils.cpp |
Implements component/native-type matching. |
lib/DxilValidation/DxilValidation.cpp |
Implements store-to-memory validation. |
docs/DXIL.rst |
Documents new validation rules. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -1225,6 +1226,66 @@ static void ValidateLinAlgMatrixStoreToDescriptor(CallInst *CI, | |||
| static void ValidateLinAlgMatrixStoreToMemory(CallInst *CI, | |||
| ValidationContext &ValCtx) { | |||
| ValidateLinAlgOpParameters(CI, ValCtx); | |||
| GEPOperator *GSGEP = cast<GEPOperator>(Op.get_memory()); | ||
| GlobalVariable *GSMem = cast<GlobalVariable>(GSGEP->getPointerOperand()); |
There was a problem hiding this comment.
I think this comment is technically correct in that we don't disallow such a formation, but I'm not actually sure it is possible to generate such code from the frontend.
| GlobalVariable *GSMem = cast<GlobalVariable>(GSGEP->getPointerOperand()); | ||
| Type *GSMemInnerTy = GSMem->getType(); | ||
| unsigned GSScalarCount = 1; | ||
| if (PointerType *GSMemPtrTy = dyn_cast<PointerType>(GSMemInnerTy)) |
There was a problem hiding this comment.
Consider making this an unconditional cast ? Or assert if this fails?
|
This was ported over from #8824 going to copy comments over |
| CI, ValidationRule::InstrLinAlgMatrixScopeMismatch2, | ||
| {"Input", MatrixScopeToString(Mat->Scope), "Wave", "ThreadGroup"}); | ||
|
|
||
| GEPOperator *GSGEP = cast<GEPOperator>(Op.get_memory()); |
There was a problem hiding this comment.
Chris B (@llvm-beanz) I'm pretty sure there is a better way to pull the inner most type out from the memory operator here but I just wanted to get something written down to unblock progress.
Does this seem right or should I do something else here. also can we even assume its always a GEP? Copilot seems to say no but I'm not sure what else it would be
There was a problem hiding this comment.
You can simplify it a bit using getSequentialElementType(). Something like:
while(SequentialType* ST = dyn_cast<SequentialType>(GSMemInnerTy))
GSMemInnerTy = ST->getSequentialElementType();
That will walk the types up because getSequentialElementType works for pointers, arrays, and vectors.
| unsigned GSScalarCount = 1; | ||
| if (PointerType *GSMemPtrTy = dyn_cast<PointerType>(GSMemInnerTy)) | ||
| GSMemInnerTy = GSMemPtrTy->getPointerElementType(); | ||
| if (ArrayType *GSMemArrTy = dyn_cast<ArrayType>(GSMemInnerTy)) { |
There was a problem hiding this comment.
From Alex: Should we check if nested arrarys are permitted?
There was a problem hiding this comment.
this will be resolved by the loop version Chris provided above
| return OS.str(); | ||
| } | ||
|
|
||
| bool IsComponentTypeSameNativeType(DXIL::ComponentType CT, llvm::Type *Ty) { |
There was a problem hiding this comment.
Chris B (@llvm-beanz) I think this is necessary since we don't really have a mapping for ComponentType to llvm::Type but figured I'd specifically highlight it since imo it's not trivially correct
There was a problem hiding this comment.
seems reasonable to me
Damyan Pepper (damyanp)
left a comment
There was a problem hiding this comment.
LGTM, but I suspect you will want to get an answer to https://github.com/microsoft/DirectXShaderCompiler/pull/8834/changes#r3867347681 (or address it in a follow-up.)
Chris B (llvm-beanz)
left a comment
There was a problem hiding this comment.
Left a few comments. Take or leave the feedback. The only one that probably matters a bit is that maybe looping to get the sequential element type is a good changes since it does handle more cases and is simpler.
| CI, ValidationRule::InstrLinAlgMatrixScopeMismatch2, | ||
| {"Input", MatrixScopeToString(Mat->Scope), "Wave", "ThreadGroup"}); | ||
|
|
||
| GEPOperator *GSGEP = cast<GEPOperator>(Op.get_memory()); |
There was a problem hiding this comment.
You can simplify it a bit using getSequentialElementType(). Something like:
while(SequentialType* ST = dyn_cast<SequentialType>(GSMemInnerTy))
GSMemInnerTy = ST->getSequentialElementType();
That will walk the types up because getSequentialElementType works for pointers, arrays, and vectors.
| return OS.str(); | ||
| } | ||
|
|
||
| bool IsComponentTypeSameNativeType(DXIL::ComponentType CT, llvm::Type *Ty) { |
There was a problem hiding this comment.
seems reasonable to me
| GEPOperator *GSGEP = cast<GEPOperator>(Op.get_memory()); | ||
| GlobalVariable *GSMem = cast<GlobalVariable>(GSGEP->getPointerOperand()); |
There was a problem hiding this comment.
I think this comment is technically correct in that we don't disallow such a formation, but I'm not actually sure it is possible to generate such code from the frontend.
|
Perfect! That's exactly the kind of feedback I was looking for:) I'm going to go ahead and merge the PR so I can make progress on the stack but I'll fix this as a follow up once everything goes in. (This change applies to a few of the stacked PRs) |
Fixes #8498
Implements LinAlg MatrixStoreToMemory validation rules
Stack created with GitHub Stacks CLI • Give Feedback 💬