[HLSL] Add LinAlg matrix accumulation contention coverage - #8818
Open
Jack Elliott (JoeCitizen) wants to merge 2 commits into
Open
[HLSL] Add LinAlg matrix accumulation contention coverage#8818Jack Elliott (JoeCitizen) wants to merge 2 commits into
Jack Elliott (JoeCitizen) wants to merge 2 commits into
Conversation
The existing matrix accumulation tests run a single Wave in a single group, so they demonstrate that an accumulation lands but never that it lands atomically. An implementation that dropped or duplicated concurrent accumulations would pass all of them. These four cases run several Waves across several groups against one destination. Each Wave contributes a distinct amount rather than the same one. Wave w repeats its additions w plus one times in the descriptor cases, and scales the value it accumulates by w plus one in the group-shared cases, so the expected total is weighted by Wave index. Had every Wave contributed the same amount the total would only pin the number of additions applied, and a dropped update from one Wave would be cancelled exactly by a duplicated update from another. Weighting also rejects a lowering that assumes the participating Waves hold the same operand and applies one representative value scaled by the participant count. Both shaders already selected a single Wave with a hardcoded index test, so the change is to compare against an ACTIVE_WAVE_COUNT define instead and let the existing helpers take a Wave count and a dispatch width. The existing callers pass one Wave and one group, which is what they did before. The group-shared oracle is now computed per element as InitialValue + WaveWeightSum * (AccumulateStartingValue + Index), where WaveWeightSum is the sum of one through the active Wave count. Both that and the descriptor count reduce to their previous values when one Wave is active, so the two existing cases are unchanged. The counts are chosen so every intermediate value is exactly representable, which is what lets the result be compared for equality rather than within a tolerance. The descriptor cases accumulate a fill of seven or one across eighty additions, and the group-shared cases reach at most three hundred and twenty-seven, both well inside the range where F16 represents integers exactly. Because every partial sum is exact, the F32 case cannot depend on the order the hardware happens to apply the additions, which is what OrderInvariant in its name refers to. The group-shared helper previously rejected anything that was not F16 through one compound condition covering four unrelated parameters. It now accepts I32 as well and reports each rejected parameter separately, so a mistake in a future case says which parameter was wrong. Verified by making every Wave contribute the same amount while leaving the oracles expecting the weighted total: the two cases that run on WARP fail, and every other test is unaffected, including the two single-Wave cases the weighting is designed not to disturb. The two I32 cases skip on WARP because it reports no I32 accumulation support. This does not prove the Waves physically overlap in time. Nothing forces an implementation to run them concurrently, so one that serialises them passes. WARP reports a Wave size of four, so the contention depth actually exercised here is low and real depth is untested until this runs on hardware. 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 contention coverage for atomic LinAlg matrix accumulation across multiple Waves and groups.
Changes:
- Adds four descriptor/group-shared contention tests.
- Uses weighted Wave contributions and expanded F16/I32 oracles.
- Preserves existing single-Wave behavior.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The descriptor cases varied how many times each Wave added the same matrix rather than what each Wave added. Every one of the additions was therefore identical, so the total pinned only how many were applied and a dropped update could still be cancelled exactly by a duplicated one. That is the property the cases were added to reject, so they did not reject it. Each Wave now scales what it accumulates and issues a fixed two additions. The scale is one plus a global index built from SV_GroupID and the Wave index, so it is distinct across every Wave in every group and the total pins which Waves landed. A Wave index on its own would repeat in each group, and an update dropped in one group could be cancelled by a duplicate in another. The group-shared shader already scaled the value it accumulated rather than repeating it, and its destination is group-shared so contention is confined to one group where Wave indices are already distinct. It is unchanged. The oracle becomes the fill times two times the sum of one through the number of contending Waves, which is sixteen Waves and so 272 times the fill. The I32 case reaches 1904 and the F32 case 272, and every partial sum along the way is a smaller integer, so both stay exact and the results can still be compared for equality. One Wave in one group gives a scale of one and two additions, which is what the expression produced before, so the existing single-Wave case is unchanged by construction rather than by a preprocessor guard. Verified by dropping the SV_GroupID term so the scale repeats in each group: the F32 contention case reaches 80 times the fill instead of 272 and fails, while the single-Wave case is unaffected because its scale is one either way. The I32 contention case skips on WARP, which reports no I32 accumulation support. Reported by Copilot code review on the pull request. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b Assisted-by: GitHub Copilot
Comment on lines
+7896
to
+7899
| runGroupSharedAccumulate(Device, DxcSupport, Params, Memory, | ||
| /*InitialValue=*/7, | ||
| /*AccumulateStartingValue=*/1, Verbose, | ||
| SelectedWaveSize, ActiveWaveCount); |
Damyan Pepper (damyanp)
left a comment
Damyan Pepper (damyanp)
left a comment
Member
There was a problem hiding this comment.
This LGTM, but I'll leave approval to someone who understands the domain better than I do.
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 four matrix accumulation contention cases that run several Waves across several groups against one destination, where the existing cases run a single Wave in a single group. Each Wave contributes a distinct amount, so the expected total is weighted by Wave index rather than only pinning how many additions were applied. The two existing single-Wave cases are unchanged by construction, since the weighted totals reduce to their previous values when one Wave is active.
Assisted-by: GitHub Copilot