[HLSL] Add thread-scope MatVec semantics coverage - #8879
Open
Jack Elliott (JoeCitizen) wants to merge 1 commit into
Open
[HLSL] Add thread-scope MatVec semantics coverage#8879Jack Elliott (JoeCitizen) wants to merge 1 commit into
Jack Elliott (JoeCitizen) wants to merge 1 commit into
Conversation
Thread-scope matrix operations are per-thread, so every lane must load its own
matrix and multiply against its own vector. The existing thread-scope cases
give every thread identical data, so an implementation that loads one matrix
and broadcasts it to the whole wave passes them. Neither case here can pass
that way.
MatVecMul_Thread_4x8_F16_PerThread gives each of 8 threads a distinct 4x8
matrix in its own 128-byte aligned slot, with element (t,m,c) = Z + c where
Z = 4t + m + 1. Vector A is {1..8}, so the expected row result is the closed
form 36*Z + 168 over sum(c+1) = 36 and sum(c*(c+1)) = 168.
MatVecMul_Thread_4x8_F16_Divergent runs the same data under non-uniform
control flow, with each arm multiplying by a different vector. Vector B is all
-1, giving -(8*Z + 28) over sum(c) = 28. The two arms are therefore disjoint by
sign, so no arm-A result can be confused with an arm-B result anywhere in the
output. An earlier revision used all +1, which collided in two places
(A(Z=1) = B(Z=22) = 204 and A(Z=3) = B(Z=31) = 276), so a swap of those slots
would have been invisible. Because the arm is chosen by lane parity, any thread
may legally land on either arm, so the portable bounds are A in [204, 1320] and
B in [-284, -36]. The largest single product is 312 and every partial sum is
bounded by its own final magnitude, so all inputs, products and results are
exactly representable in F16.
The divergent case branches on WaveGetLaneIndex() rather than
SV_GroupThreadID. Thread parity only guarantees that different threads take
different arms; the distribution of threads to waves is implementation defined
(hlsl-specs 0048-group-wave-index.md), so an implementation that segregates
parities into separate waves would make both arms wave-uniform and the case
would prove nothing while still passing. Lane parity guarantees divergence
inside the wave.
Because that guarantee still depends on the wave containing more than one
active thread, each thread records a witness word before the branch holding
its predicate bit and a WaveActiveAnyTrue vote in both directions. The host
checks that at least one thread reports a wave that executed both arms.
If none does, the case reports Skipped rather than Failed. Implementations may
launch additional waves, those waves may contain inactive lanes, and the
distribution of threads among them is implementation defined, so a wave holding
only same-parity lanes is legal. Failing there would reject a conformant
implementation for a precondition this test cannot itself guarantee. The
per-thread results are still verified first and a wrong value still fails, so
the downgrade applies only when nothing else is wrong.
Each arm also writes a branch-local marker. The host requires the marker to
agree with the pre-branch witness, and validates results against the witness
rather than the marker. Validating against the marker would be circular: an
implementation that executed the wrong arm would write that arm's marker, and
the host would then check it against the oracle for the arm that actually ran
instead of the one the predicate selected.
Assisted-by: GitHub Copilot
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
Copilot started reviewing on behalf of
Jack Elliott (JoeCitizen)
September 3, 2026 19:18
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The divergent test must skip devices without WaveOps support before shader compilation or execution.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds thread-scoped matrix-vector multiplication tests using per-thread data and divergent control flow.
Changes:
- Adds distinct per-thread F16 matrices with CPU-oracle validation.
- Adds lane-parity divergence coverage with execution witnesses.
- Requires a WaveOps capability guard for the divergent test.
- Includes two style nits requesting explicit return types instead of
auto.
File summaries
| File | Description | Findings |
|---|---|---|
tools/clang/unittests/HLSLExec/LinAlgTests.cpp |
Adds shaders, fixtures, verification, and test cases. | 1 moderate WaveOps guard issue (2 votes); 2 explicit-type nits (1 vote each). |
Review details
Suppressed comments (2)
tools/clang/unittests/HLSLExec/LinAlgTests.cpp:7347
- Use the explicit return type here per the project's almost-never-
autostyle.createComputeOpreturns the straightforwardstd::unique_ptr<st::ShaderOp>type (HlslExecTestUtils.h:611-614).
auto Op = createComputeOp(Shader, "cs_6_10", "SRV(t0), SRV(t1), UAV(u2)",
Args.c_str());
tools/clang/unittests/HLSLExec/LinAlgTests.cpp:7355
- Use the explicit return type here per the project's almost-never-
autostyle.runShaderOpreturnsstd::shared_ptr<st::ShaderOpTestResult>(HlslExecTestUtils.h:640-645), which remains clear enough to spell out.
auto Result = runShaderOp(
- 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.
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.
Thread-scope matrix operations are per-thread, so every lane must load its own matrix and multiply against its own vector, but the existing thread-scope cases give every thread identical data and cannot distinguish that from an implementation that loads one matrix and broadcasts it across the wave. These two cases give each of eight threads a distinct 4x8 matrix in its own aligned slot, checked against a closed-form CPU oracle, and the second runs the same data under non-uniform control flow with each arm using a different vector. It branches on lane index rather than thread index, because thread-to-wave distribution is implementation defined and only lane parity makes the arms diverge inside a wave; each thread records a pre-branch witness so a run where that never happened is reported inconclusive rather than passed or failed.
Assisted-by: GitHub Copilot