[HLSL] Derive LinAlg ThreadGroup matrix shapes from the device wave size - #8817
Open
Jack Elliott (JoeCitizen) wants to merge 3 commits into
Open
[HLSL] Derive LinAlg ThreadGroup matrix shapes from the device wave size#8817Jack Elliott (JoeCitizen) wants to merge 3 commits into
Jack Elliott (JoeCitizen) wants to merge 3 commits into
Conversation
The three ThreadGroup matrix arithmetic tests used hardcoded 8x16x8 and 8x8x8 shapes. WARP reports a minimum wave size of four, so those extents happen to satisfy its shape rule and the tests pass. A device reporting a minimum wave of 16, 32 or 64 finds no usable wave size at all, logs "No executable ThreadGroupMatrixMultiply configuration" and skips. The tests would therefore have run on the software rasterizer and quietly skipped on the hardware they exist to qualify. Shapes are now derived from the wave size the capability query selects: M and N are the smallest multiple of the wave size that is at least the plan's minimum extent, and K is a fixed multiple of M. At wave size four this reproduces the previous 8x16x8 and 8x8x8 shapes exactly, so the existing validation carries over unchanged. The derived shape must also keep the F16 accumulator exact. The value patterns bound the operands to |a| <= 2 and |b| <= 3, so a dot product cannot exceed 6K. Half precision represents integers exactly below 2048, and the largest shape the policy can produce caps the sum at 1536. ThreadGroupShapePolicy is a host self-test covering wave sizes four through 128, including the 32 and 64 that no software rasterizer can exercise. It asserts the derived extents divide the wave size, that K stays a multiple of four, that the extent chosen is the smallest legal one, and that the accumulator bound above still holds. Pinning the derivation back to the old wave-four behaviour makes it fail on the divisibility assertion, so it catches the defect this change fixes. The three tests are renamed because their dimensions are no longer fixed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b Assisted-by: GitHub Copilot
Adversarial review of the wave-derived shape change found two ways it could stay quiet when it should not. A plan with a zero minimum extent or a zero K multiple derived an invalid case for every wave size. The selector skipped each one in turn, then reported that no configuration was supported, which routes to a capability-gated skip. An authoring mistake therefore looked exactly like a device that lacks the feature, on every device. The parent revision caught this because it asserted case validity before the capability query, and deriving the shape later lost that. The plan is now validated up front and a degenerate one fails, while a derived case that is somehow still invalid returns a hard error rather than moving to the next wave size. The shape self-test only covered a multiplying plan with a K multiple of two, so a derivation that hardcoded either would have passed. That matters most for the operation: losing it would compile the accumulating test without its accumulator and drop the same term from the oracle, so the test would agree with itself and pass without exercising accumulation. The self-test now runs the three plans the tests actually use, checks the operation and accumulation survive derivation, includes the accumulator in the exactness bound, and adds a minimum extent that is neither a multiple nor a divisor of the wave size. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b Assisted-by: GitHub Copilot
Contributor
There was a problem hiding this comment.
Pull request overview
Updates ThreadGroup matrix tests to derive legal dimensions from device wave size.
Changes:
- Introduces wave-scaled matrix test plans and selection.
- Renames three ThreadGroup tests.
- Adds shape-policy self-tests.
Suppressed comments (1)
tools/clang/unittests/HLSLExec/LinAlgTests.cpp:5626
- The condition checks that each derived extent is a multiple of the wave size, so this failure message states the relationship backwards.
"Derived extents must divide the wave size");
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
Real hardware may also support 128. I haven't checked the code, but given this comment I'm wondering if there are assumptions that would mean the 128 isn't handled correctly? |
The shape rule is that each extent must be a multiple of the wave size, which is what the derivation and the assertion actually check: M is rounded up to a multiple of the wave size, and the self-test asserts M % WaveSize == 0. Two places stated the relationship the other way round, saying the extents divide the wave size, and the plan comment then contradicted itself one sentence later by describing the rule correctly. The failure message is corrected the same way, so a self-test failure describes the rule that was broken rather than its inverse. A second comment described the self-test as reaching 32 and 64, which reads as though 64 were the ceiling. The language allows 128, and the sweep has always covered it, so the comment now says so and a point assertion pins the largest wave explicitly. It is written against hlsl::DXIL::kMaxWaveSize rather than a literal so it follows the constant if that ever changes. Verified by capping the derivation at 64 and re-running: the self-test fails on the divisibility check at 128, so the coverage is real rather than incidental. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b Assisted-by: GitHub Copilot
Collaborator
Author
Verified that 128 is covered, update the description |
Damyan Pepper (damyanp)
approved these changes
Aug 23, 2026
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.
The ThreadGroup matrix tests added in #8813 use fixed extents. ThreadGroup matrix operations require the extents to divide the device's wave size, and WARP reports waves of 4 to 16 while real hardware reports 32, 64 or 128. An 8x16x8 shape is legal on WARP and illegal on a 64-wide wave, so those three tests pass in CI and silently skip on the hardware the HLK is meant to qualify.