[HLSL] Add out-of-bounds store coverage for LinAlg descriptor stores - #8769
Merged
Jack Elliott (JoeCitizen) merged 3 commits intoAug 13, 2026
Merged
Conversation
Proposal 0035 requires MatrixStoreToDescriptor to bounds check its writes, permitting either of two behaviours: drop the whole store if any element falls outside the descriptor view, or drop only the elements that fall outside. The suite already covers the load side of that rule. Nothing covered the store side, so an implementation that wrote past its descriptor view passed. Adds two tests that store a matrix through a destination view shorter than its buffer, mirroring the two existing load cases. One uses a packed 16x16 F16 matrix with a 260 byte view, admitting 130 of 256 elements. The other uses a 4x8 F16 matrix at a 128 byte offset with a 172 byte view, admitting 14 of 32. Both boundaries fall inside a row rather than on a row or padding edge. The comparison is byte level rather than matrix level. Both permitted behaviours leave the bytes past the view unwritten, holding the poison the destination was seeded with, so there is no element value to compare against; decoding those bytes as F16 can also produce NaN, which does not compare equal to itself. Two host helpers support this: storeBufferBoundedByView derives the expected bytes for a given view, and verifyStoreBuffer reports the first differing byte for each candidate. The source is viewed in full so only the destination is bounds checked, leaving the result attributable to the store alone. The runner also asserts that the chosen view both admits and excludes at least one whole element, since a view that did neither would accept any result. These cases cannot by themselves fail an implementation that stores nothing at all, because dropping the whole store is one of the two permitted behaviours. The existing LoadStoreDescriptor cases, which view the destination in full, are what require the store to happen. The 260 byte boundary is deliberate. An earlier draft used 264, which admits 132 elements. That is exactly 33 times 4, so on a device with a 64 lane wave and four elements per lane, an implementation that bounds checked once per lane rather than once per element would see every lane as wholly inside or wholly outside and produce output identical to per-element checking. Per-lane checking is not one of the two permitted behaviours, so that would have been a false pass. 130 is not a multiple of the per-lane element count at any wave size the tile supports below 128 lanes. Validated on WARP: 36 total, 31 passed, 4 failed, 1 skipped, with the non-passing set identical to the branch baseline of 33/28/4/1. Two controls confirm the tests detect what they claim. Widening the destination view to the full buffer fails both, reporting the first differing byte at 260, exactly the view boundary. Making the host oracle write zeroes instead of restoring poison fails the oracle self-test, which the previous value-only check would have passed silently. 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
Adds conformance coverage for Proposal 0035’s out-of-bounds descriptor store behavior.
Changes:
- Adds byte-level store-result oracles with poison preservation.
- Tests packed and offset/padded partial destination views.
- Adds CPU coverage for view-boundary handling.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Damyan Pepper (damyanp)
approved these changes
Aug 12, 2026
Damyan Pepper (damyanp)
left a comment
Damyan Pepper (damyanp)
left a comment
Member
There was a problem hiding this comment.
LGTM, would be good to get more domain expertise looking at it.
Alex Sepkowski (alsepkow)
approved these changes
Aug 13, 2026
Co-authored-by: Alex Sepkowski <alexsepkowski@gmail.com>
Contributor
You can test this locally with the following command:git-clang-format --diff bc0a474927ed1d8018854ead41a33616aed4a4c6 c9f2f5ea189174eaf1cd1bd13fa02ca6cdaaadc9 -- tools/clang/unittests/HLSLExec/LinAlgTests.cppView the diff from clang-format here.diff --git a/tools/clang/unittests/HLSLExec/LinAlgTests.cpp b/tools/clang/unittests/HLSLExec/LinAlgTests.cpp
index c6750a62..c077552a 100644
--- a/tools/clang/unittests/HLSLExec/LinAlgTests.cpp
+++ b/tools/clang/unittests/HLSLExec/LinAlgTests.cpp
@@ -1326,8 +1326,8 @@ static bool verifyStoreBuffer(const void *ActualBuffer, size_t ActualBufferSize,
const std::wstring &PublicRule, bool Verbose) {
if (Candidates.size() < 2 || PublicRule.empty()) {
hlsl_test::LogErrorFmt(
- L"Invalid store buffer oracle: candidates=%zu, public rule is %s",
- Candidates.size(), PublicRule.empty() ? L"empty" : L"present");
+ L"Invalid store buffer oracle: candidates=%zu, public rule is %s",
+ Candidates.size(), PublicRule.empty() ? L"empty" : L"present");
return false;
}
|
Contributor
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 (1)
tools/clang/unittests/HLSLExec/LinAlgTests.cpp:1329
- This continuation line is misindented relative to the surrounding formatted calls. Reindent the format string so this hunk remains clang-formatted.
L"Invalid store buffer oracle: candidates=%zu, public rule is %s",
Alex Sepkowski (alsepkow)
approved these changes
Aug 13, 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.
Proposal 0035 requires
MatrixStoreToDescriptorto bounds check its writes, permitting either dropping the whole store if any element falls outside the descriptor view, or dropping only the elements that fall outside.These two tests store through a destination view shorter than its buffer, mirroring the existing load cases.