[HLSL] Fix LinAlg stride and group-shared offset preconditions - #8865
Merged
Jack Elliott (JoeCitizen) merged 3 commits intoSep 1, 2026
Merged
Conversation
Proposal 0035 requires that the Stride argument of a matrix Load on a [RW]ByteAddressBuffer be a multiple of 16 bytes. matrixStrideBytes returned the tightly packed stride, so three positive tests were driving the shader with an 8-byte descriptor stride: MatVecMul_Thread_4x8_F16_ColumnMajor 4 * 2 bytes = 8 MatVecMul_Thread_4x8_I8_Interpreted 8 * 1 bytes = 8 MatVecMul_Thread_4x8_U8_Interpreted 8 * 1 bytes = 8 These pass today, but the proposal does not define runtime behaviour once the precondition is violated, so a pass cannot be used as conformance evidence and a stricter implementation could fail them at any time. Round the stride up to a multiple of 16 in matrixStrideBytes, which is the single value feeding the host buffer encoding, the buffer size and the -DMATRIX_STRIDE shader argument, so all three stay consistent by construction. Padding only inserts zero gaps between rows or columns; calculateExpected indexes the logical matrix values and never consults the stride, so no oracle changes. The other three MatVec cases are already 16-byte aligned and are unaffected: U32 4x8 RowMajor is 32, FP8 4x16 is 16, and F16 4x8 RowMajor is 16. Also add the stride postcondition to isCaseValid, which was deliberately left out while the I8 and U8 cases still used a stride of 8. Negative controls, both predicted before running. Removing the padding while keeping the postcondition failed exactly the three cases above and nothing else, confirming they were the only violators. Keeping the padded shader stride while encoding the host buffer at the packed stride also failed exactly those three, confirming the padded stride reaches the shader and the buffer is really laid out at the new stride. Per-test differential against 8320476 with no outcome changes in either configuration: released SDK 78/73/0/5, preview SDK 78/74/2/2. The two preview failures are the pre-existing toLinAlgDataType FP8 gap, unrelated to this change. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
Proposal 0035 requires a matrix stride in memory to be a multiple of 16
bytes, and requires the start of a group-shared matrix to be 128-byte
aligned. Seven more positive tests were driving out-of-contract layouts:
CopyConvert_Wave_4x8_F32_ToF16_Transpose destination stride 8
LoadStoreMemory_Wave_4x8_F16_RowMajorOffsetPadded offset 8, stride 24
and canonical stride 8
LoadStoreMemory_Wave_4x8_F32_ColumnMajorOffsetPadded offset 16, stride 24
LoadStoreMemory_ThreadGroup_4x8_F16 offset 8, stride 12
AccumulateMemory_Wave_16x16_F16 padded subcase offset 8, stride 24
AccumulateMemoryContention_Wave_4x8_F16 offset 8, stride 24
AccumulateMemoryContention_Wave_4x8_I32 offset 16, stride 40
Round MatrixParams::strideBytes up to a multiple of 16, which covers the
CopyConvert destination and every other descriptor stride derived from it,
and keeps returning 0 for the optimal layouts where the proposal requires
it. Move the alignment constants next to it so the MatVec helper, the
group-shared gate and the existing DescriptorAlignedOffset all share one
definition instead of repeating the literals.
Re-pad the group-shared layouts so each one still exercises a padded stride
and a non-zero start while satisfying the preconditions, and enforce both
rules in getGroupSharedBufferDescription, which every group-shared Load,
Store and Accumulate path already funnels through. That turns a silent
out-of-contract layout into an immediate failure and stops this class of
violation being reintroduced.
The oracles follow the layout rather than the other way around: buffer
sizes, the host encoding and the shader offset and stride arguments are all
derived from the same MatrixBufferLayout, so re-padding needs no separate
expectation update.
The unaligned layouts remaining in LinAlgCPUOracleTests are deliberate.
Those are host-only tests of writeMatrixBuffer and verifyMatrixBuffer
padding behaviour, they run no shader, and the DXIL preconditions do not
apply to them.
Negative controls, both predicted before running. Reverting the ThreadGroup
target offset to 8 failed exactly that test with "Invalid group-shared
buffer description", confirming the new gate is live. Keeping the padded
DST_STRIDE while encoding the host destination at the packed stride failed
exactly CopyConvert_Wave_4x8_F32_ToF16_Transpose, confirming the padded
stride reaches the shader.
Per-test differential against 8320476 with no outcome changes in either
configuration: released SDK 78/73/0/5, preview SDK 78/74/2/2. abicheck
EXITCODE=0.
AccumulateMemoryContention_Wave_4x8_I32 is capability-skipped on the test
device, so its new layout is compiled but not executed here.
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
Updates linear algebra execution tests to comply with Proposal 0035 alignment requirements.
Changes:
- Aligns matrix strides to 16 bytes.
- Aligns group-shared matrix offsets to 128 bytes.
- Rejects invalid group-shared layouts.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Alex Sepkowski (alsepkow)
approved these changes
Aug 31, 2026
Alex Sepkowski (alsepkow)
left a comment
Alex Sepkowski (alsepkow)
left a comment
Contributor
There was a problem hiding this comment.
LGTM with one minor comment.
The previous commit introduced MatrixStrideAlignmentBytes, MatrixOffsetAlignmentBytes and alignMatrixStride(), but only runGroupSharedAccumulateContention actually used them. The four other group-shared layouts it updated kept hand-computed literals, so the file carried both the rule and a set of numbers that happened to satisfy it, with nothing tying the two together. Each padded offset is now MatrixOffsetAlignmentBytes, and each stride is derived from the matrix that uses it: alignMatrixStride() of the minor extent times the component size for a tight-but-aligned stride, plus one or two MatrixStrideAlignmentBytes units where the case deliberately pads. That makes the intent readable at the use site - a reader can now see which layouts are packed and which are padded without recomputing element sizes - and it keeps the strides legal if a case ever changes shape or component type. Zero offsets are left as literals. Proposal 0035 constrains the alignment of a non-zero matrix start, so a zero there means "no offset" rather than an alignment choice, and naming it after the alignment constant would misdescribe it. Two nearby groups of literals are deliberately unchanged. The descriptor layouts already route their offsets through DescriptorAlignedOffset and carry comments deriving their strides, and they are not touched by this change. The LinAlgCPUOracleTests layouts use offsets and strides such as 4 and 12 precisely because they are not legal GPU matrix strides; they exercise the host codec's arithmetic, and forcing them onto 16- and 128-byte boundaries would delete that coverage. No value changes. Every substitution was checked against the literal it replaced by static_assert in a standalone translation unit, and that check was verified non-vacuous by injecting two wrong expectations and confirming both failed to compile. On preview WARP the suite is Total=78, Passed=74, Failed=2, Skipped=2, with zero per-test differences against the parent commit across all 78 methods. The two failures are the FP8 conversion cases, which need the type mapping in microsoft#8866 and are unrelated to this change. Addresses review feedback from @alsepkow. 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 1, 2026 00:28
View session
Collaborator
|
FYI: there is a related spec change coming around this in response to microsoft/hlsl-specs#922. |
Damyan Pepper (damyanp)
approved these changes
Sep 1, 2026
Damyan Pepper (damyanp)
left a comment
Damyan Pepper (damyanp)
left a comment
Member
There was a problem hiding this comment.
Ashley Coleman (@V-FEXrt) - should we have expected the validator catch this at this point, or is validation for this not in main yet?
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 requires linear algebra matrix memory strides to be a multiple of 16 bytes and group-shared matrix starts to be 128-byte aligned, but ten cases were passing illegal operands and only passing because WARP is more permissive than the specification requires.
Assisted-by: GitHub Copilot