Skip to content

[SM6.10] LinAlg Validation: MatrixStoreToMemory - #8834

Merged
Ashley Coleman (V-FEXrt) merged 1 commit into
mainfrom
linalg-vali-matrixstoretomemory
Sep 1, 2026
Merged

[SM6.10] LinAlg Validation: MatrixStoreToMemory#8834
Ashley Coleman (V-FEXrt) merged 1 commit into
mainfrom
linalg-vali-matrixstoretomemory

Conversation

@V-FEXrt

Copy link
Copy Markdown
Collaborator

Fixes #8498

Implements LinAlg MatrixStoreToMemory validation rules


Stack created with GitHub Stacks CLIGive Feedback 💬

Fixes #8498

Implements LinAlg MatrixStoreToMemory validation rules
Copilot AI balanced review requested due to automatic review settings August 26, 2026 17:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Comment on lines +1242 to +1243
GEPOperator *GSGEP = cast<GEPOperator>(Op.get_memory());
GlobalVariable *GSMem = cast<GlobalVariable>(GSGEP->getPointerOperand());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making this an unconditional cast ? Or assert if this fails?

@V-FEXrt

Copy link
Copy Markdown
Collaborator Author

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());

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Alex: Should we check if nested arrarys are permitted?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will be resolved by the loop version Chris provided above

return OS.str();
}

bool IsComponentTypeSameNativeType(DXIL::ComponentType CT, llvm::Type *Ty) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems reasonable to me

@damyanp Damyan Pepper (damyanp) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

@llvm-beanz Chris B (llvm-beanz) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems reasonable to me

Comment on lines +1242 to +1243
GEPOperator *GSGEP = cast<GEPOperator>(Op.get_memory());
GlobalVariable *GSMem = cast<GlobalVariable>(GSGEP->getPointerOperand());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@V-FEXrt

Copy link
Copy Markdown
Collaborator Author

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)

@V-FEXrt
Ashley Coleman (V-FEXrt) merged commit fe09c22 into main Sep 1, 2026
17 checks passed
@V-FEXrt
Ashley Coleman (V-FEXrt) deleted the linalg-vali-matrixstoretomemory branch September 1, 2026 21:35
@github-project-automation github-project-automation Bot moved this from New to Done in HLSL Roadmap Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

LinAlg Validation: MatrixStoreToMemory

5 participants