[SM6.10] LinAlg Validation: MatrixLoadFromMemory - #8836
Open
Ashley Coleman (V-FEXrt) wants to merge 1 commit into
Open
[SM6.10] LinAlg Validation: MatrixLoadFromMemory#8836Ashley Coleman (V-FEXrt) wants to merge 1 commit into
Ashley Coleman (V-FEXrt) wants to merge 1 commit into
Conversation
Damyan Pepper (damyanp)
left a comment
Member
There was a problem hiding this comment.
LGTM - although this also looks extremely similar to the MatrixStoreToMemory version.
Ashley Coleman (V-FEXrt)
force-pushed
the
linalg-vali-matrixloadfrommemory
branch
from
September 1, 2026 21:35
29da409 to
73b4d5d
Compare
Copilot started reviewing on behalf of
Ashley Coleman (V-FEXrt)
September 1, 2026 21:36
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Pointer handling can crash the validator, while bounds and alignment calculations use incorrect memory units.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds SM 6.10 validation for LinAlg matrix loads from groupshared memory.
Changes:
- Validates matrix scope, storage type, capacity, offset, and stride.
- Adds DXIL validation coverage.
- Updates code-generation fixtures.
File summaries
| File | Description |
|---|---|
lib/DxilValidation/DxilValidation.cpp |
Implements load validation rules. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-matrixloadfrommemory.ll |
Adds validator tests. |
tools/clang/test/CodeGenDXIL/hlsl/linalg/builtins/matrixloadfrommemory/nominal.hlsl |
Updates scalar-array expectations. |
tools/clang/test/CodeGenDXIL/hlsl/linalg/builtins/matrixloadfrommemory/vector-array.hlsl |
Updates vector-array expectations. |
Review details
Suppressed comments (3)
lib/DxilValidation/DxilValidation.cpp:1643
- This is not the memory footprint of the read: it ignores the GEP/start offset, runtime
Offset,Stride, and row/column layout, and it also undercounts 64-bit matrices stored throughi32(for example, a 4x4 I64 matrix needs 32 i32 slots, not 16). As a result, the new test's “okay” load starts at element 128 of a 64-element global and is accepted. Compute the last byte/slot touched from the resolved base, storage element width, offset, stride, layout, and matrix dimensions, then compare that footprint with the global allocation.
unsigned ElementsPerScalar = ComponentTypeElementsPerScalar(RetMat->Type);
unsigned ExpectedScalarCount =
(RetMat->N + ElementsPerScalar - 1) / ElementsPerScalar * RetMat->M;
if (ExpectedScalarCount > GSScalarCount)
lib/DxilValidation/DxilValidation.cpp:1653
Offsetis an element index, not a byte count: the public API names itStartIdx, andLinAlgTests.cpp:7896dividesOffsetBytesby the element size before passing it. Checking the raw index against 128 rejects valid offsets such as 32 forfloat(128 bytes). Convert the index to a byte offset using the memory pointee's allocation size before enforcing 128-byte alignment.
if (ConstantInt *OffsetV = dyn_cast<ConstantInt>(Op.get_offset())) {
unsigned Offset = OffsetV->getLimitedValue();
if (Offset % 128 != 0)
ValCtx.EmitInstrFormatError(CI, ValidationRule::InstrParamMultiple,
{"Offset", "128", std::to_string(Offset)});
lib/DxilValidation/DxilValidation.cpp:1661
- Like
Offset, the groupsharedStrideis expressed in memory elements (LinAlgTests.cpp:7897passesStrideBytes / ElementBytes). Applying% 16directly rejects a valid float stride of 4 elements (16 bytes), and is even more restrictive for vector arrays. Scale by the pointee allocation size before checking the 16-byte requirement.
if (ConstantInt *StrideV = dyn_cast<ConstantInt>(Op.get_stride())) {
unsigned Stride = StrideV->getLimitedValue();
if (Stride % 16 != 0)
ValCtx.EmitInstrFormatError(CI, ValidationRule::InstrParamMultiple,
{"Stride", "16", std::to_string(Stride)});
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Base automatically changed from
linalg-vali-matrixaccumulatetomemory
to
main
September 2, 2026 15:58
Fixes #8499 Implements LinAlg MatrixLoadFromMemory validation rules
Ashley Coleman (V-FEXrt)
force-pushed
the
linalg-vali-matrixloadfrommemory
branch
from
September 2, 2026 15:59
73b4d5d to
e385cd2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8499
Implements LinAlg MatrixLoadFromMemory validation rules
Stack created with GitHub Stacks CLI • Give Feedback 💬