[HLSL] Add group-shared matrix I/O coverage for LinAlg - #8773
Open
Jack Elliott (JoeCitizen) wants to merge 2 commits into
Open
[HLSL] Add group-shared matrix I/O coverage for LinAlg#8773Jack Elliott (JoeCitizen) wants to merge 2 commits into
Jack Elliott (JoeCitizen) wants to merge 2 commits into
Conversation
Synchronize the existing group-shared load, store, and accumulation baselines so every thread participates in typed initialisation, both group barriers, and race-free UAV readback. Exercise Wave F16 row-major, Wave F32 column-major, and ThreadGroup F16 transfers in both directions between different layouts. Non-zero offsets, padded strides, and typed guard elements make consistent addressing permutations and adjacent-memory corruption observable. Build exact whole-buffer expectations on the CPU from logical values 1 through 32. The padded F16 atomic case derives the same values from MatrixGetCoordinate, accumulates them into 12, and therefore expects 13 through 44 while preserving every guard element. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
Contributor
There was a problem hiding this comment.
Pull request overview
Adds robust group-shared LinAlg matrix I/O execution coverage.
Changes:
- Makes baseline initialization/readback cooperative and barrier-safe.
- Adds padded, offset, bidirectional Wave and ThreadGroup transfer tests.
- Adds capability-gated group-shared accumulation coverage with guard validation.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Jack Elliott (JoeCitizen)
enabled auto-merge (squash)
August 15, 2026 00:35
Take the review suggestion to break the seven-term condition in getGroupSharedBufferDescription into sequential checks. The chain was hard to read and the split generates the same optimised code, so nothing is lost by writing it out plainly. Also drop the parentheses around numeric_limits and use uint32_t instead of the UINT macro. The parentheses were guarding against a max macro that is not active in this translation unit, and UINT is the same type, so the bound is unchanged. Two comments differ from the suggestion because the checks they described are divisibility tests against the element size rather than non-zero tests, and the final bound admits an element count equal to the uint32_t maximum. The dereference of RequiredBytes is retained because the optional cannot be used directly as the left operand of the modulo. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tools/clang/unittests/HLSLExec/LinAlgTests.cpp:5142
- This 4x8 subcase is reached only after the 16x16 construction checks succeed. Matrix-construction support is shape-specific (the existing guidance at lines 3174-3180 requires querying smaller shapes independently), so a device may support this 4x8 case while rejecting 16x16, causing the new coverage to be silently skipped. Run the independently gated helper before the 16x16 early returns (or register it as a separate test).
runPaddedGroupSharedAccumulateCase(D3DDevice, DxcSupport, VerboseLogging);
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.
Proposal 0035 specifies group-shared matrix load, store and accumulate. The existing baselines set up and read back their group-shared buffers without every thread crossing both barriers, and their initialisation loop divided the element count by the thread count, so it initialised nothing whenever the group was larger than the matrix.
Setup and readback are now cooperative grid-stride loops, and three transfers are added: Wave F16 row-major, Wave F32 column-major and ThreadGroup F16. Each runs in both directions between differing layouts with non-zero offsets, padded strides and typed guard elements, so a matching address permutation cannot cancel and adjacent corruption stays observable. Expected whole-buffer contents are built on the CPU from logical values 1 through 32.
The gated 4x8 atomic subcase is not reachable on WARP today.