Skip to content

[SM6.10] Implement LinAlg PSV0 runtime data tracking - #8893

Open
Ashley Coleman (V-FEXrt) wants to merge 4 commits into
microsoft:mainfrom
V-FEXrt:linalg-psv0-2
Open

[SM6.10] Implement LinAlg PSV0 runtime data tracking#8893
Ashley Coleman (V-FEXrt) wants to merge 4 commits into
microsoft:mainfrom
V-FEXrt:linalg-psv0-2

Conversation

@V-FEXrt

Copy link
Copy Markdown
Collaborator

Fixes #7843

Declares, collects, dumps and verifies PSV0 runtime data. Commits are split up such that its easiest to review this PR commit by commit.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Add the PSV0 LinAlg runtime record layout and teach the PSV reader and writer to serialize the optional tables.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

LinAlg metadata can be omitted or forged during validation, and malformed PSV data can trigger out-of-bounds reads.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds Shader Model 6.10 Linear Algebra runtime metadata to PSV0.

Changes:

  • Defines and serializes LinAlg runtime records.
  • Collects, dumps, and validates LinAlg metadata.
  • Adds coverage for LinAlg operations and absent metadata.
File summaries
File Description
include/dxc/DxilContainer/DxilPipelineStateValidation.h Defines LinAlg PSV structures and serialization.
lib/DxilContainer/DxilContainerAssembler.cpp Collects LinAlg usage from DXIL.
lib/DxilContainer/DxilPipelineStateValidation.cpp Dumps LinAlg PSV data.
lib/DxilValidation/DxilContainerValidation.cpp Parses and validates LinAlg records.
tools/clang/test/DXC/dumpPSV_LinAlg.hlsl Tests combined LinAlg metadata.
tools/clang/test/DXC/dumpPSV_LinAlgAccumulate.hlsl Tests accumulation records.
tools/clang/test/DXC/dumpPSV_LinAlgConstructions.hlsl Tests construction records.
tools/clang/test/DXC/dumpPSV_LinAlgMatVec.hlsl Tests matrix-vector records.
tools/clang/test/DXC/dumpPSV_LinAlgMatrixMultiply.hlsl Tests matrix-multiply records.
tools/clang/test/DXC/dumpPSV_AS.hlsl Checks absent LinAlg metadata.
tools/clang/test/DXC/dumpPSV_CS.hlsl Checks absent LinAlg metadata.
tools/clang/test/DXC/dumpPSV_DS.hlsl Checks absent LinAlg metadata.
tools/clang/test/DXC/dumpPSV_GS.hlsl Checks absent LinAlg metadata.
tools/clang/test/DXC/dumpPSV_HS.hlsl Checks absent LinAlg metadata.
tools/clang/test/DXC/dumpPSV_MS.hlsl Checks absent LinAlg metadata.
tools/clang/test/DXC/dumpPSV_PS.hlsl Checks absent LinAlg metadata.
tools/clang/test/DXC/dumpPSV_VS.hlsl Checks absent LinAlg metadata.
Review details
  • Files reviewed: 17/17 changed files
  • Comments generated: 6
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/DxilContainer/DxilPipelineStateValidation.cpp
Comment thread lib/DxilValidation/DxilContainerValidation.cpp Outdated
Comment thread lib/DxilValidation/DxilContainerValidation.cpp
Comment thread lib/DxilValidation/DxilContainerValidation.cpp Outdated
Comment thread lib/DxilValidation/DxilContainerValidation.cpp Outdated
Comment on lines +178 to +182
enum class PSVRuntimeInfo4Flag : uint32_t {
None = 0x00000000,
// Indicates use of LinAlg operations beyond the Tier 1 required set, thus
// the presence of the PSVLinAlgRuntimeInfo structure with usage details.
LinAlgRuntimeInfoPresent = 0x00000001,

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

LinAlg metadata can bypass module-content validation, and malformed PSV data introduces out-of-bounds reads.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

lib/DxilValidation/DxilContainerValidation.cpp:470

  • This verifier never checks that the LinAlg metadata matches the DXIL module: an absent flag returns successfully, and present records are checked only for shape-index validity. A container can therefore omit the extension or alter operation types/counts/flags while still passing validation, causing the runtime to receive capabilities that do not describe the shader. Regenerate the expected LinAlg data from DM (as is done for other PSV content) and compare the presence flag and every table.
void PSVContentVerifier::VerifyLinAlgRuntimeInfo() {
  if (!PSV.GetPSVLinAlgRuntimeInfo0())

lib/DxilValidation/DxilContainerValidation.cpp:475

  • ShapeRef.Count is untrusted, but MarkUse checks Offset + Size, which can wrap. A crafted large count can pass that check and the loop below then reads ShapeIndexes[I] beyond the semantic-index table. Validate the count with subtraction before calling MarkUse.
  auto VerifyShapes = [&](const PSVLinAlgMatrixShapeArrayReference &ShapeRef) {
    if (!IndexTableVerifier.MarkUse(ShapeRef.ShapesIndex, ShapeRef.Count)) {
      EmitInvalidError("LinAlgOperationShapes");

lib/DxilValidation/DxilContainerValidation.cpp:798

  • Each record-size word is read before checking that it is present. If a nonzero table count is followed by a truncated blob, validation dereferences past the end of pPSVData instead of rejecting it safely.
        uint32_t RecordSize = GetUint32AtOffset(pPSVData, Offset);
        INCREMENT_POS(4);

lib/DxilContainer/DxilPipelineStateValidation.cpp:1020

  • Get validates only the starting index, not ShapeRef.Count. dxa -dumppsv invokes this printer after structural parsing but without PSVContentVerifier, so malformed metadata whose shape array extends past the semantic-index table makes Indexes[I] read out of bounds. Validate the complete range before iterating.
      const uint32_t *Indexes = m_SemanticIndexTable.Get(ShapeRef.ShapesIndex);
      for (uint32_t I = 0; I < ShapeRef.Count; ++I) {
        if (I)
          OS << ", ";
        PSVLinAlgMatrixOperationShape0 *Shape =
            Indexes ? GetPSVLinAlgMatrixOperationShape(Indexes[I]) : nullptr;

include/dxc/DxilContainer/DxilPipelineStateValidation.h:182

  • This adds user-visible runtime metadata for experimental Shader Model 6.10, so the release-note policy calls for an entry under docs/ReleaseNotes.mdUpcoming Preview ReleaseExperimental Shader Model 6.10. Please add an entry for the new LinAlg PSV0 tracking (or point to the related PR that will provide the shared entry).
enum class PSVRuntimeInfo4Flag : uint32_t {
  None = 0x00000000,
  // Indicates use of LinAlg operations beyond the Tier 1 required set, thus
  // the presence of the PSVLinAlgRuntimeInfo structure with usage details.
  LinAlgRuntimeInfoPresent = 0x00000001,
  • Files reviewed: 17/17 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread lib/DxilValidation/DxilContainerValidation.cpp Outdated
Print the optional LinAlg runtime tables and resolve their operation shape references in PSV dumps.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Validate the optional LinAlg table layout and verify that operation shape references use valid semantic-index and shape-table entries.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The serialized format breaks existing PSV4 compatibility and validation has correctness gaps.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

include/dxc/DxilContainer/DxilPipelineStateValidation.h:182

  • This adds user-visible SM 6.10 compiler/PSV behavior, so the repository release-note policy calls for an entry under docs/ReleaseNotes.md’s Upcoming Release. Please add one, or point to the related PR that will provide shared release-note coverage.
enum class PSVRuntimeInfo4Flag : uint32_t {
  None = 0x00000000,
  // Indicates use of LinAlg operations beyond the Tier 1 required set, thus
  // the presence of the PSVLinAlgRuntimeInfo structure with usage details.
  LinAlgRuntimeInfoPresent = 0x00000001,
  • Files reviewed: 17/17 changed files
  • Comments generated: 4
  • Review effort level: Balanced

Comment thread include/dxc/DxilContainer/DxilPipelineStateValidation.h
Comment thread lib/DxilContainer/DxilContainerAssembler.cpp
Comment on lines +508 to +510
bool HasLinAlgRuntimeInfo = PSV.GetPSVLinAlgRuntimeInfo0() != nullptr;
bool ExpectedHasLinAlgRuntimeInfo =
ExpectedPSV.GetPSVLinAlgRuntimeInfo0() != nullptr;
Comment on lines +532 to +535
if (!ActualRecord || !ExpectedRecord || \
memcmp(ActualRecord, ExpectedRecord, sizeof(Record)) != 0) { \
EmitMismatchError(Name, std::to_string(I), std::to_string(I)); \
break; \
Collect LinAlg construction, multiply, outer-product, and accumulate-store usage while assembling PSV0, and cover the serialized records through PSV dumping.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It introduces a new serialized compiler metadata format and validation path whose compatibility and correctness warrant final human review.

Review details
  • Files reviewed: 17/17 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

Update PSV0 for Linear Algebra

2 participants