[HLSL] Test signed-I8 groupshared loads through MMA - #8920
Open
Jack Elliott (JoeCitizen) wants to merge 3 commits into
Open
Jack Elliott (JoeCitizen) wants to merge 3 commits into
Jack Elliott (JoeCitizen) wants to merge 3 commits into
Conversation
Encode signed I8 elements as bytes in raw i32 groupshared storage, then load row-major A and column-major B using matrix-element offsets and strides. The 512-byte starts and 128/192-byte padded strides also keep a /4 unit-fault control aligned and in bounds under the current conservative validator. Feed A, B and an independent I32 accumulator from runtime SRVs. Compare all 128 result cells with the CPU integer product and check source snapshots and output guards independently. Add a hand-encoded host check for signed bytes, word packing, row/column addressing and guards. Reuse the existing construction/MMA capability intersection and force the wave size actually queried. No shared helper, compiler or ABI changes. This test-only change does not need a release note. Local checkpoint awaiting human review. The current-base WARP run exposes a new result mismatch; runtime attribution and control validation remain open. This is not a claim about physical IHV hardware. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
Collect the result of each supported forced-wave run before failing the test, rather than stopping after the first arithmetic mismatch. This keeps later advertised waves observable without weakening any capability query, matrix comparison, source-byte check or output-guard check. Local follow-up awaiting human review. The supplied WARP stack still fails the valid public-unit case at both selected wave sizes, while preserving the source bytes and output guards. Temporary unit and descriptor probes are not part of this commit and do not establish physical IHV behaviour. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
Jack Elliott (JoeCitizen)
marked this pull request as ready for review
September 14, 2026 20:00
Copilot started reviewing on behalf of
Jack Elliott (JoeCitizen)
September 14, 2026 20:01
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The encoding helper incorrectly rejects valid matrices ending on partial i32 words.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds signed-I8 group-shared matrix-load execution coverage through I32 MMA.
Changes:
- Adds byte-encoding oracle coverage.
- Adds padded row/column-major group-shared MMA execution tests.
- Verifies results, guards, and source preservation.
File summaries
| File | Description |
|---|---|
tools/clang/unittests/HLSLExec/LinAlgTests.cpp |
Adds I8 encoding and MMA tests. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Count the word containing the last matrix byte before appending whole guard words, then convert the checked word count back to bytes. Packed I8 layouts no longer have to end on an i32 boundary. Extend the existing CPU encoding case with one-, two- and three-byte tails in both layouts, comparing the full allocation including padding and guards. The existing word-aligned MMA fixture is unchanged. Address the encoding-helper feedback on PR microsoft#8920. This AI-assisted follow-up awaits human review. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d48ff7e3-e976-4c12-92fd-e039126a8243
Copilot started reviewing on behalf of
Jack Elliott (JoeCitizen)
September 14, 2026 20:55
View session
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No correctness defects were found; only a minor explicit-type convention issue remains.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tools/clang/unittests/HLSLExec/LinAlgTests.cpp:8655
- These two
autodeclarations hide a short, stable return type and conflict with the repository's almost-never-auto convention. Please spell outstd::optional<std::vector<BYTE>>; the laterOp/Resultdeclarations are reasonable exceptions because their concrete types are substantially more cumbersome.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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.
Add signed-I8 groupshared matrix-load coverage using byte-encoded i32 storage, matrix-element offsets and strides, and an independent exact I32 MMA oracle.
Refs #7841.
Assisted-by: GitHub Copilot