[HLSL] Add mandatory FP8 matrix MatVec coverage - #8840
Open
Jack Elliott (JoeCitizen) wants to merge 5 commits into
Open
[HLSL] Add mandatory FP8 matrix MatVec coverage#8840Jack Elliott (JoeCitizen) wants to merge 5 commits into
Jack Elliott (JoeCitizen) wants to merge 5 commits into
Conversation
getFP8Format and encodeHalfToFP8 currently sit roughly 7000 lines below namespace matvec_interpretation, next to their only caller in the Convert tests. The MatVec interpretation harness needs them next, so move the block up rather than forward-declare it: a forward declaration would have to duplicate the FP8Format return type, and the file already groups shared host helpers ahead of the namespaces that consume them. This is a pure relocation. No line is added, removed or edited; the sorted line multiset of the file is unchanged and the diff is a balanced 78/78. clang-format 17.0.6 reports no drift, so the move is separated from the behavioural change that follows it. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
Audit finding F1 noted that the two FP8 matrix rows of the Tier 1 vector-matrix table were never exercised. This adds Thread-scope MatVec cases for Fp16 vector x F8_E4M3FN matrix -> Fp16 and Fp16 vector x F8_E5M2 matrix -> Fp16, plus a host oracle test that pins the FP8 encoder and the new decoder against a hand-derived table. Why these cases are Mandatory, not CapabilityGated D3D12LinearAlgebraRuntimeFeatureSupport.md marks both rows Optional only under the Native column, and the note beneath the table requires an implementation that does not support them natively to emulate them. So a device reporting a linear algebra tier must accept both combinations, and CapabilityRequirement::Mandatory is the correct classification: runCapabilityChecked turns an advertised-but-unsupported combination into a Fail rather than a success-shaped Skip, while still downgrading to a Skip on a device that reports no tier at all. No change to the already-reviewed applicability policy was needed. Why the shape is 4x16 and must not be reduced to 4x8 The device matrix Load precondition in proposal 0035 requires Stride to be a multiple of 16 bytes. matrixStrideBytes returns MinorCount * componentByteSize and an FP8 component is one byte, so a RowMajor FP8 matrix needs sixteen columns to emit a legal stride. A 4x8 FP8 matrix would emit an eight-byte stride and be non-conformant. Note this does not add a stride assertion to isCaseValid, because makeSInt8Case and makeUInt8Case are eight columns of one-byte components today and would trip it. That is a separate defect with its own fix. How the expected values were derived The matrix and vector values, the encoded bytes and the expected results were derived from the FP8 format table in the specification using an independent model, not by observing shader output. The bias arithmetic was cross-checked against the Max values the specification states for each format, 0x7E -> 448 for E4M3FN and 0x7B -> 57344 for E5M2. Every matrix value is exactly representable in both formats. The expectations are exact rather than a permitted-result set. calculateExpected accumulates in int64_t, the emulation clause of 0035 requires an emulated format to represent all shader-visible values accurately, and its divergence note applies only to saturation to infinity. The largest result is 30, far below the E4M3FN maximum of 448, so native and emulated implementations must agree exactly. encodeComponents guards the FP8 path with an encode-then-decode equality check, so a value that is not exactly representable is reported through reportUnrepresentable instead of silently changing what the test asserts. Current result on WARP Both cases fail on WARP today, which is the intended conformance signal rather than a defect in the test. WARP advertises the combination through ThreadVectorMatrixMultiply, compiles and dispatches the shader, and returns values that are not integers even though every input is an exact small integer, so it is decoding bytes other than the ones supplied. Two controls establish the tests themselves are sound: the identical case with an F16 matrix, same shape and same expectations, passes; and the FP8 case fails identically at a padded 32-byte stride, so the fault is the component type and not the stride. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
Jack Elliott (JoeCitizen)
requested review from
Ashley Coleman (V-FEXrt) and
Damyan Pepper (damyanp)
August 27, 2026 19:16
Contributor
There was a problem hiding this comment.
Pull request overview
Adds mandatory thread-scope FP8 matrix-vector conformance coverage.
Changes:
- Supports FP8 matrix-value encoding and decoding.
- Adds E4M3FN/E5M2 execution cases and a golden host-oracle test.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The Min row of the FP8 format table in proposal 0035 gives S.0000.001 as 2^-9 for E4M3FN and S.00000.01 as 2^-16 for E5M2, so an exponent of zero with a nonzero mantissa is a finite subnormal rather than an invalid encoding. decodeFP8ToFloat rejected every such byte. It now decodes them without the implicit leading bit and with the scale taken at exponent one, which reproduces both values the specification states. The golden table gains the subnormal encodings for both formats, including the two Min values, a mid-ladder mantissa, negative cases and both signed zeros, so the branch is pinned rather than merely present. Reverting only the decode arm while keeping the table fails the test with "F8_E4M3FN decoding of 0x01 is 0.000000, proposal 0035 says 0.001953", so the new entries are not vacuous. This was not reachable from the MatVec harness, which is why it survived the earlier review: encodeHalfToFP8 refuses any value whose destination exponent would be zero or below, so the round-trip guard in encodeComponents never presented the decoder with a subnormal, and CaseData::MatrixValues is integral so the smallest nonzero magnitude any matrix case can request is one. The decoder is nevertheless wrong on its own terms against the specification, and it takes an arbitrary byte rather than only bytes this file produced. The encoder is deliberately left refusing to emit subnormals. Its refusal is fail-safe, surfacing through reportUnrepresentable rather than a silently wrong byte, and widening it would require round-to- nearest-even into the subnormal grid plus acceptance of F16 subnormal inputs, since the E5M2 minimum is itself subnormal in F16. That is a larger and riskier change than this one and does not belong in a coverage PR. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
Supersedes the "Current result on WARP" section of 23e3b21 and the matching claim in the pull request description. Both stated that the two FP8 matrix MatVec tests failed because of an implementation defect. That attribution was wrong: the tests themselves were invalid. WARP reports FLOAT8_E4M3FN and FLOAT8_E5M2 vector-matrix multiplication with flags 0x7, that is SUPPORTED | EMULATED_INPUTS | EMULATED_OUTPUTS. Under EMULATED_INPUTS the runtime feature specification requires the matrix to be loaded from a MulOptimal layout, so a RowMajor load is not a legal request. The tests were asking for something the specification does not permit and reading the refusal as a defect. A MulOptimal buffer cannot be produced on the host. Proposal 0035 permits matrix stores only at Wave or ThreadGroup scope with a RowMajor or ColumnMajor layout, and the byte layout is implementation defined, so the runtime conversion API is the only way to obtain one. The matrix is therefore staged as RowMajor source bytes and converted on the device by ConvertLinearAlgebraMatrix immediately before the dispatch. Two constraints fell out of the DXIL validator and are worth recording, because the OuterProduct precedent in this file does not satisfy either and copying it directly does not work: - LinAlgMatrixLoadFromDescriptor requires a stride of zero for optimal layouts. A nominal stride is legal for MatrixAccumulateToDescriptor, which is why the OuterProduct path passes one, but it is rejected for a load. - A Thread scope load requires a read only ByteAddressBuffer, so the destination is bound as an SRV. It is transitioned to UNORDERED_ACCESS for the conversion and back again, and the closing transition also orders the conversion write before the load. CaseData gains a LoadFromMulOptimal flag rather than setting Layout to MulOptimal, because Layout drives the host encoder and the case validity check, both of which treat any non RowMajor value as ColumnMajor. Layout continues to describe the source bytes and shaderLayout() describes what the shader loads. The conversion runs unconditionally for these two cases rather than being selected on the reported EMULATED_INPUTS flag. Proposal 0035 permits an optimal layout for a Thread scope load in all cases and requires it under emulation, so one deterministic path is both correct everywhere and easier to review than behaviour that branches on a capability bit. The oracle is unchanged. Conversion is layout only, the source and destination component types are identical, and every matrix value is drawn from a set that is exact in both FP8 formats, so the expected output remains [-19, 11, 30, -6]. The conversion API is not exposed by the released Windows SDK, so both tests are gated on DIRECT3D_LINEAR_ALGEBRA and skip in an ordinary build and in CI, matching the existing OuterProduct_Thread_16x16_F16 behaviour. Under _HLK_CONF the missing API is reported as an error instead. ShaderOpTest gains a pre-dispatch command callback so work can be recorded on the dispatch list before the pipeline state is set. This mirrors the existing post-dispatch callback and is additive; every existing caller is unaffected. Validation on WARP, comparing per test outcomes rather than totals: - preview SDK build, before and after: exactly two changes, both FP8 MatVec tests moving from Failed to Passed, no other test changing bucket (78 total, 72 to 74 passed). - released SDK build, before and after: the same two tests moving from Failed to Skipped, no other test changing bucket, and no remaining failures (78 total, 73 passed, 0 failed, 5 skipped). The two Convert_F16_To{E4M3FN,E5M2}_AndBack failures in the preview build are pre-existing on main and unrelated: toLinAlgDataType has no FP8 cases, so those tests fail while logging a datatype. They are untouched by this change. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
Damyan Pepper (damyanp)
left a comment
Damyan Pepper (damyanp)
left a comment
Member
There was a problem hiding this comment.
Reviewed style and comments. Not checked the tests in detail.
Addresses two review comments on microsoft#8840. Delete the MATRIX_STRIDE comment. The zero stride required for opaque optimal layouts is a DXIL specification requirement, not a DXIL validator restriction, so the comment misattributed the constraint while adding nothing the expression does not already say. Move the zero destination-size check out of the MatVec caller and into getLinAlgMatrixByteSize. GetLinearAlgebraMatrixConversionDestinationInfo returns void and signals a request it cannot serve by leaving DestSize zero, so every caller needs the same check. That is our own wrapper, so the check belongs inside it rather than being restated, and commented on, at each call site. This also hardens the two pre-existing runOuterProduct call sites, which performed no check at all and would have silently converted into a zero-byte destination. Negative control: forcing DestSize to zero inside the helper failed MatVecMul_Thread_4x16_F8_E4M3FN, MatVecMul_Thread_4x16_F8_E5M2 and OuterProduct_Thread_16x16_F16, confirming the assertion is live and now covers a path that previously had none. Validation, per-test differential against c4d917d with no outcome changes in either configuration: preview SDK 78/74/2/2, released SDK 78/73/0/5. abicheck EXITCODE=0. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
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 Thread-scope MatVec coverage for the two FP8 matrix rows of the Tier 1 vector-matrix table (Fp16 vector times F8_E4M3FN and F8_E5M2 matrices, Fp16 result), classified Mandatory because the runtime documentation requires those combinations to be emulated where they are not supported natively, plus a host oracle test pinning the FP8 encoder and the new decoder against a hand-derived table.