[HLSL] Add MatVec interpretation and bias coverage for LinAlg - #8775
[HLSL] Add MatVec interpretation and bias coverage for LinAlg#8775Jack Elliott (JoeCitizen) wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds thread-scope LinAlg matrix-vector tests for layouts, interpreted inputs, unsigned output, and independent bias.
Changes:
- Adds six
Multiply/MultiplyAddexecution tests. - Adds host-side encoding, overflow-checked oracles, capability checks, and guard-byte validation.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
baa671f to
2010c0e
Compare
Damyan Pepper (damyanp)
left a comment
There was a problem hiding this comment.
Some observations, none of these should block the PR after it has been reviewed by someone with more domain knowledge than me.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tools/clang/unittests/HLSLExec/LinAlgTests.cpp:2287
- Please use the explicit result type under the repository's almost-never-
autoconvention.runShaderOpdeclares this asstd::shared_ptr<st::ShaderOpTestResult>inHlslExecTestUtils.h:601.
auto Result =
tools/clang/unittests/HLSLExec/LinAlgTests.cpp:2272
- Per the repository's almost-never-
autoconvention, this straightforward return type should be explicit.createComputeOpis declared as returningstd::unique_ptr<st::ShaderOp>inHlslExecTestUtils.h:571.
This issue also appears on line 2287 of the same file.
auto Op = createComputeOp(Shader, "cs_6_10", RootSignature, Args->c_str());
Exercise non-uniform F16 row-major and column-major layouts, packed SInt8 and UInt8 interpreted inputs, unsigned UInt32 output, and a separate non-uniform bias resource. High-bit UInt8 lanes distinguish unsigned from signed decoding of the same bytes. Derive every expected result with overflow-checked host dot products plus optional bias, encode packed lanes least-significant-byte first, and compare the complete poisoned output including padding and guard bytes. Query the exact vector, matrix, bias, and result capability tuple, requiring both mandatory F16 layouts and gating only optional output cases. Column-major is required rather than capability gated because a thread scope matrix load permits row-major, column-major and optimal layouts, and only transposed loads are implementation specific and need a driver query. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
LinAlgTests.cpp places helper namespaces before the test classes that use them: cpu_oracle sits at the top of the file, ahead of the first test class. matvec_interpretation was appended after the classes instead, which left no way for a LinAlgCPUOracleTests method to call into it without a forward declaration. This is a pure relocation of the namespace block. No line is added, removed or edited: the file has the same 6060 lines before and after, and the sorted set of lines is identical. clang-format reports no drift. Verified with the full HLSLExec LinAlg selection on WARP, compared per test rather than by totals: 45 total, 39 passed, 5 failed, 1 skipped, with the non-passing set unchanged from the parent commit. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
Move the checked int64 helpers into cpu_oracle, next to the existing checkedMultiply and checkedAdd for size_t. That namespace is already the file's home for arithmetic the oracle relies on, so matvec_interpretation no longer carries its own copies. The checks stay: encodeComponents accepts UInt32 values up to 4294967295, so a UInt32 matrix times a UInt32 vector overflows int64 by construction. An overflow in the oracle yields a wrong expected value, which can pass a broken implementation rather than fail a correct one, so this is the direction worth guarding. Report the rejected value when a component cannot be represented in the target type. All six rejection sites now name the type and the value instead of failing with no context, and componentTypeName covers SInt8 and UInt8 rather than returning "Unsupported" for them. Promote the oracle self-test out of runCase into its own method on LinAlgCPUOracleTests, so it runs once instead of once per case. That class already exists for exactly this purpose and deliberately carries no Kits metadata, so HLK runs never select it. Each assertion gets its own VERIFY, replacing a single six-term conjunction that could not say which part failed. Verified with the full HLSLExec LinAlg selection on WARP, compared per test rather than by totals: 46 total, 40 passed, 5 failed, 1 skipped. The only difference from the parent commit is the added MatVecHostOracle passing; the non-passing set is unchanged. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
Follows the same review feedback Damyan and Chris gave on microsoft#8774, applied here so the two pull requests stay consistent and so neither lands this code. Every value these helpers guarded is authored by the test: dimensions are literals of at most M=16 and N=16, and the largest integer literal in the file is 65504. The widest accumulation the oracle can perform is far below the int64 range, so the overflow branches were unreachable, and an overflow would have indicated a bug in the test rather than a driver failing conformance. checkedAddInt64 and checkedMultiplyInt64 are removed together with the two self-test assertions that existed only to exercise them. calculateExpected returns its result directly rather than an optional and reads as ordinary arithmetic. Its size preconditions are already established by isCaseValid, which runCase verifies before any of this is reached. This also removes a collision that neither pull request shows in its own diff. microsoft#8774 defines the same two helpers in the same cpu_oracle namespace but in a different region of the file, so git would have merged both without conflict and left main with a duplicate definition. Verified with the full HLSLExec LinAlg selection on WARP, compared per test rather than by totals: 46 total, 40 passed, 5 failed, 1 skipped, identical to the parent commit. Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
c89b779 to
9b8b5b5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tools/clang/unittests/HLSLExec/LinAlgTests.cpp:1924
- The host oracle performs the dot product with unchecked signed
int64_tmultiplication and addition. A valid supported case using largeU32operands can therefore trigger signed-overflow undefined behavior and produce unreliable expected bytes, contrary to the PR description's overflow-checked oracle claim. Please use checked or wider arithmetic and reject results that cannot be represented before encoding them.
Expected[Row] +=
Case.MatrixValues[static_cast<size_t>(Row) * Case.N + Column] *
Case.InterpretedVectorValues[Column];
tools/clang/unittests/HLSLExec/LinAlgTests.cpp:1811
- This round-trip check can itself invoke undefined behavior:
INT64_MAXrounds to2^63when converted tofloat, and converting that out-of-range float back toint64_tis undefined. Check thatFloatValueis within the half-openint64_trange before performing the cast, then do the exactness comparison.
This issue also appears on line 1922 of the same file.
if (static_cast<int64_t>(FloatValue) != Value)
return reportUnrepresentable(Type, Value);
Damyan Pepper (damyanp)
left a comment
There was a problem hiding this comment.
LGTM - my previous comments are all addressed.
Proposal 0035 permits a thread-scope
Multiplyvector to be native or anInterpretedVector, pairing a packed vector with an interpretation type. The interpreted forms had no coverage.These six tests cover non-uniform F16 in row-major and column-major layouts, packed SInt8 and UInt8 inputs, unsigned UInt32 output, and
MultiplyAddwith the bias in a separate resource. The UInt8 case reuses the SInt8 bytes so high-bit lanes decode differently under the two interpretations. Expected values come from overflow-checked host dot products, and the full output allocation including padding and guard bytes is compared against a poison fill.Column-major is required rather than gated: a thread-scope load permits row-major, column-major and optimal layouts, and only transposed loads need a driver query.
MatVecMulAdd_Thread_4x8_F16_IndependentBiasfails on WARP, which advertises the F16 tuple then rejectsdx.op.linAlgMatVecMulAddas an invalid overload, matching two existing failures on the merge base.