[HLSL] Add LinAlg vector accumulation contention coverage - #8825
Open
Jack Elliott (JoeCitizen) wants to merge 2 commits into
Open
[HLSL] Add LinAlg vector accumulation contention coverage#8825Jack Elliott (JoeCitizen) wants to merge 2 commits into
Jack Elliott (JoeCitizen) wants to merge 2 commits into
Conversation
The vector accumulation tests dispatch a single thread, so they show that an accumulation lands but not that concurrent accumulations all land. An implementation that dropped or duplicated updates under contention would pass every one of them. These two cases dispatch sixty-four threads in each of four groups at one destination. The threads do not all add the same vector. One component varies with the invocation index modulo four, so the expected total depends on which invocations landed and not only on how many did. Had every invocation added the same vector, a dropped update would be cancelled exactly by a duplicated one, and a lowering that assumes the threads hold the same operand and applies one representative vector scaled by the thread count would also match. Varying one component rejects both. The helper already built a NUMTHREADS define through buildCompilerArgs and its shader ignored it, and createComputeOp already accepted a dispatch width. So the change is to use the define the shader was already being given and to pass the width through. Both new parameters default to a single thread in a single group, which is what the three existing callers already did. The varying component is derived from SV_DispatchThreadID, which is zero for those callers, so they are unchanged. The expected values are written out rather than computed so a reviewer can check them against the rule by eye. Invocation t adds 1 + t modulo 4, 2, 3 and 4 to the first four elements. Elements one to three gain 256 * (I + 1), and element zero gains 256 + 384 = 640 because each of the four residues occurs sixty-four times. The F16 case runs from ten to thirteen and reaches 13 + 1024 = 1037; the F32 case runs from twenty to twenty-three and reaches 23 + 1024 = 1047. Every partial sum along the way is a smaller integer, so all of them are exact in both formats and the results can be compared for equality. Because no partial sum rounds, the F32 result cannot depend on the order the hardware applies the additions, which is what OrderInvariant in its name refers to. Both cases carry two guard elements past the accumulated range. Those are pinned by the exact comparison, not by the untouched-byte check, which treats every byte of the destination matrix as its own and so covers the poison below the start offset rather than the guards. Since the expected values are derived for a fixed invocation count, each case asserts that count, and that the count divides evenly by the variation, rather than trusting the constants to stay consistent with the literals. Verified by removing the varying component so every invocation adds the same vector: element zero reaches 266 rather than 650, both new cases fail on that element alone, and the three existing cases are unaffected because their single invocation never varied. Passing at exactly two hundred and fifty-six accumulations also requires both the thread count and the dispatch width to be wired through, since either alone gives a different total. This does not prove the threads physically overlap in time, so an implementation that serialises them passes. 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
Adds multi-group contention coverage for LinAlg vector descriptor accumulation.
Changes:
- Parameterizes thread and dispatch counts.
- Adds exact F16 and order-invariant F32 contention tests.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The contention shader varied only element zero, as DispatchThreadID.x modulo four, so the 256 invocations produced just four distinct contributed vectors with 64 copies each. Dropping one invocation's update and applying another's twice therefore left the accumulated sum bit-identical whenever the two shared a residue, and an exhaustive search over ordered pairs finds 16128 such compensating pairs. The comment above the tests claimed the opposite. Each vector component now carries one digit of the invocation index in base THREAD_VARIATION. Four components at base four gives exactly the 256 invocations dispatched, so the contribution is a bijection of the invocation index and the same search finds no compensating pairs. Every digit value occurs 64 times in each position, so element I accumulates 256 * (I + 1) + 384. The largest result is 1421, which keeps every partial sum exactly representable in F16 and so preserves the order invariance the F32 case asserts. The single-threaded callers are unaffected because invocation zero contributes every digit as zero. A discriminating negative control forced one defect against both encodings: dropping invocation zero and duplicating invocation four fails under the base digit encoding, reporting 908 against an expected 907 at component one, and passes unnoticed under the previous encoding. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
Comment on lines
+8108
to
+8110
| // The base each vector component carries one digit of the invocation index in, | ||
| // so contending threads all accumulate distinct vectors. A single-threaded | ||
| // dispatch always has invocation zero and so is unaffected. |
There was a problem hiding this comment.
Seems that there may be a name other than VectorAccumulateVariation that more accurately describes what it is for?
Damyan Pepper (damyanp)
approved these changes
Aug 25, 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.
Adds two vector accumulation contention cases that dispatch sixty-four threads in each of four groups against one destination, where the existing cases dispatch a single thread. One component of each contributed vector varies with the invocation index, so the expected total depends on which invocations landed rather than only on how many did. The three existing cases are unchanged by construction, since the varying component derives from SV_DispatchThreadID and is zero for a single invocation.
Assisted-by: GitHub Copilot