[PIX] Fix shared instrumentation resource handling by damyanp · Pull Request #8842 · microsoft/DirectXShaderCompiler

Damyan Pepper (damyanp) · 2026-08-27T22:44:48Z

Part 2 of 14 in the PIX instrumentation stack. It targets users/damyanp/pix-fixes-01, which supplies the validation helpers. Eleven later layers use the helpers this layer changes, so please review it with care.

More than one PIX pass can run on the same module. Each pass adds its own tools UAV, so a pipeline with two passes makes two resources at the same register and space.

The code that adds this UAV to a root signature has three unsafe paths. Root signature serialization can fail and give a null blob, which the code then reads. An empty result can replace a correct root signature. Only the first global root-signature subobject gets the new parameter, so a state object that declares more than one is left in a mixed state.

The tools UAV is a raw buffer, so it changes the shader flags of the module. The code does not compute the flags again. Some passes make a dx.op overload declaration and then find no callers for it. They leave the declaration in the module, and the validator refuses a module that has an unused declaration.

When serialization fails, the helpers keep the original root signature. A valid signature is better than an empty one.

Assisted-by: Copilot

This changes only the PIX instrumentation, so it needs no release note.

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

The PIX passes change DXIL after the compiler completes. A pass can add a resource, change a root signature, insert an operation, and remove a declaration. The result can be a module that the validator refuses. The tests only examine the disassembly, so they cannot find this type of defect. The new helpers run the DXIL validator on the output of a pass. They also separate the diagnostics that PIX instrumentation is permitted to cause from the diagnostics that are defects. The set of permitted diagnostics decides what every later layer can ignore, so it is the part to examine with care. There is no change to the compiler. Assisted-by: Copilot Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 40dc9de3-617e-4caf-ab0d-fba0a033ed93

More than one PIX pass can run on the same module. Each pass adds its own tools UAV, so a pipeline with two passes makes two resources at the same register and space. The code that adds this UAV to a root signature has three unsafe paths. Root signature serialization can fail and give a null blob, which the code then reads. An empty result can replace a correct root signature. Only the first global root-signature subobject gets the new parameter, so a state object that declares more than one is left in a mixed state. The tools UAV is a raw buffer, so it changes the shader flags of the module. The code does not compute the flags again. Some passes make a dx.op overload declaration and then find no callers for it. They leave the declaration in the module, and the validator refuses a module that has an unused declaration. When serialization fails, the helpers keep the original root signature. A valid signature is better than an empty one. Assisted-by: Copilot Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 40dc9de3-617e-4caf-ab0d-fba0a033ed93

Damyan Pepper (damyanp) · 2026-08-27T23:27:42Z

/azp run

azure-pipelines · 2026-08-27T23:27:49Z

Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions.

Copilot

Pull request overview

Fixes PIX instrumentation resource reuse, root-signature handling, shader flags, and unused DXIL operation declarations.

Changes:

Reuses tools UAVs and safely updates all global root signatures.
Recomputes shader flags and centralizes unused declaration cleanup.
Adds regression and validation coverage for affected PIX passes.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`tools/clang/unittests/HLSL/PixTest.cpp`	Adds PIX resource, root-signature, flags, cleanup, and validation tests.
`tools/clang/test/HLSLFileCheck/pix/pixelCounterEarlyZ.hlsl`	Updates expected raw-buffer shader flags.
`lib/DxilPIXPasses/PixPassHelpers.h`	Exposes the operation cleanup helper.
`lib/DxilPIXPasses/PixPassHelpers.cpp`	Implements UAV reuse, root-signature updates, flags, and cleanup.
`lib/DxilPIXPasses/DxilRemoveDiscards.cpp`	Removes unused discard declarations.
`lib/DxilPIXPasses/DxilPIXMeshShaderOutputInstrumentation.cpp`	Cleans obsolete mesh operation overloads.
`lib/DxilPIXPasses/DxilPIXAddTidToAmplificationShaderPayload.cpp`	Cleans the replaced dispatch declaration.
`lib/DxilPIXPasses/DxilOutputColorBecomesConstant.cpp`	Cleans unused output overloads.
`lib/DxilPIXPasses/DxilNonUniformResourceIndexInstrumentation.cpp`	Cleans unused instrumentation operations.
`lib/DxilPIXPasses/DxilDebugInstrumentation.cpp`	Removes obsolete UAV-selection logic.
`lib/DxilPIXPasses/DxilDebugBreakInstrumentation.cpp`	Uses centralized operation cleanup.

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

Virtual-register annotation attaches metadata that DXIL does not consume, so validation of an instrumented module always reported a generic "unused metadata" diagnostic even when the module was otherwise correct. The prior handling matched that diagnostic by substring, which would also swallow a genuinely unrelated unused-metadata defect. Replace it with a structural check: on direct validation failure, clone the module, strip only the four known PIX virtual-register metadata kinds, and revalidate. Accept only if the stripped clone validates, proving PIX metadata was the sole cause. Rework Validation_ControlInvalidModuleFails so the corrupted container itself proves both facts independently: direct validation's diagnostic confirms the permitted PIX metadata is present and unused, and the harness's rejection confirms a real, non-boilerplate defect remains. Replace Validation_ControlBoilerplateOnlyFailureIsRejected, which exercised the removed string classifier, with Validation_ControlNonPixUnusedMetadataIsRejected: a foreign metadata kind alongside the module's own PIX metadata must still be rejected after the four-kind strip. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Apply the repository's almost-never-auto convention to every remaining auto introduced by the L1 original and direct-feedback commits. Use the declared Compile and RunSinglePass result types and std::string::size_type for the find result. No behavior changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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

Bring the final approved L1 tip, including its formatting follow-up, into this layer while preserving the original L2 commit as the first parent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

ExtendRootSig returns early, without appending a parameter, when a version 1.1 root signature already holds the requested tools UAV. The caller still unconditionally cleared Descriptor.Flags on the last parameter in that case, which may be an unrelated, pre-existing parameter whose flags the caller must not disturb. Have ExtendRootSig report whether it actually appended a parameter, and only clear flags on that path. Add a regression test: a v1.1 root signature with the tools UAV already present as parameter 0 and an unrelated root descriptor as parameter 1 with DataVolatile flags. Re-requesting the same register leaves parameter 1's flags untouched; appending a genuinely new register still leaves parameter 1 untouched and gives the new parameter None flags. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Replace the stable, nameable auto-deduced types introduced by L2. Retain only the two DxilSubobjects MapVector ranges whose underlying pair type is implementation-dependent and obscures the loop. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

Declaration cleanup mutations must be reflected in pass modification results.

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

Review details

Suppressed comments (1)

tools/clang/unittests/HLSL/PixTest.cpp:4243

Use the explicit CComPtr<IDxcBlob> type here. Compile results use that type throughout these new tests (for example, line 4224 immediately above), matching the repository's almost-never-auto convention.

  auto compiled = Compile(m_dllSupport, source, L"cs_6_10", {});

Files reviewed: 11/11 changed files
Comments generated: 1
Review effort level: Balanced

Use Capitalized names for the parameters and local variables added by the PIX validation harness. Remove Hungarian prefixes without changing older code in PixTest.cpp. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 819bcd16-83fc-4619-ac1a-b12ee50a2fe8

Rename the PIX validation helpers to verb phrases that start with a lowercase letter, as required by the LLVM coding standards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 819bcd16-83fc-4619-ac1a-b12ee50a2fe8

Bring the approved L1 naming sequence forward without rewriting published L2. Resolve the single helper conflict by retaining L2's dxilemit option while using L1's renamed parameter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Rename only the C++ declarations introduced by L2 and their required references. Preserve framework test names, established LLVM abbreviations, pre-existing identifiers, and embedded HLSL fixture content. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Apply the explicitly approved naming exception to ExtendRootSig's existing parameter without changing any other pre-existing identifier. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

Two critical validation issues remain in mesh declaration cleanup and shader-flag recomputation.

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

Review details

Files reviewed: 11/11 changed files
Comments generated: 2
Review effort level: Balanced

Reject known PIX metadata with an invalid attachment location or payload before removing it for DXIL validation. Add controls for function metadata, metadata on the wrong instruction type, and malformed metadata. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 819bcd16-83fc-4619-ac1a-b12ee50a2fe8

Bring the approved L1 metadata placement and payload validation forward without rewriting published L2. The merge is conflict-free and introduces no L2-specific logic. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

Mesh shader instrumentation can leave an unused emitIndices declaration and fail validation.

Review details

Suppressed comments (2)

lib/DxilPIXPasses/DxilPIXMeshShaderOutputInstrumentation.cpp:384

dx.op.emitIndices is also materialized unconditionally above, but this cleanup only tracks the storeVertexOutput overloads. Valid mesh shaders can call SetMeshOutputCounts(0, 0) without writing their indices output (for example, shader_targets/mesh/vertices_sig_bigger_than_primitives_sig_regression.hlsl), so instrumenting one still leaves an unused emitIndices declaration and can fail validation. Preserve that function pointer and run eraseIfUnused on it as well.

  SmallVector<Function *, 4> StoreVertexOutputFunctions;

lib/DxilPIXPasses/PixPassHelpers.h:51

eraseIfUnused is the only exported helper in this interface that uses lower camel case; the surrounding helpers (CreateGlobalUAVResource, GetEntryFunction, GetAllInstrumentableFunctions, etc.) use UpperCamelCase. Please rename this to EraseIfUnused and update its callers so the public helper API remains consistent.

void eraseIfUnused(hlsl::DxilModule &DM, llvm::Function *OpFunction);

Files reviewed: 11/11 changed files
Comments generated: 0 new
Review effort level: Balanced

Return whether PIX operation declarations were erased and propagate that state through no-op pass paths and dynamic-resource traversal. Keep cleanup-only execution from emitting instrumentation diagnostics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Keep the materialized emitIndices function and erase it when a valid zero-output mesh shader has no index calls. Add a validating regression for that path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Recompute shader flags after non-uniform-index instrumentation and before metadata emission. Normalize bare optimizer bitcode to assert serialized WaveOps and validate unqualified paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🟢 Approval recommended

The correctness fixes are coherent and well tested; only non-blocking explicit-type style feedback remains.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

lib/DxilPIXPasses/PixPassHelpers.cpp:204

The repository's stated almost-never-auto convention applies here: the dependent pointee type is still short and explicit, and spelling it out also makes the parameter-array ownership clearer.

This issue also appears on line 217 of the same file.

lib/DxilPIXPasses/PixPassHelpers.cpp:217

Please use the explicit pointer type here per the repository's almost-never-auto convention; RootParameterDesc * is concise and avoids hiding the allocation type.

  auto *newParams = new RootParameterDesc[RootSignatureDesc.NumParameters + 1];

Files reviewed: 12/12 changed files
Comments generated: 0 new
Review effort level: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 819bcd16-83fc-4619-ac1a-b12ee50a2fe8

Bring the finalized L1 tip based on current main forward without rewriting published L2. The merge is conflict-free and adds no L2-specific logic. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

Root-signature replacement still discards optional source-text metadata.

Get a fresh assessment by requesting another Copilot review.

Review details

Files reviewed: 12/12 changed files
Comments generated: 1
Review effort level: Balanced

@@ -267,16 +285,24 @@ static void AddUAVToDxilDefinedGlobalRootSignatures(DxilModule &DM) {
        constexpr bool notALocalRS = false;
        if (subObject.second->GetRootSignature(notALocalRS, Data, Size,
                                               nullptr)) {
-          auto extendedRootSig = AddUAVParamterToRootSignature(Data, Size);
-          auto rootSignatureSubObjectName = subObject.first;
-          subObjects->RemoveSubobject(rootSignatureSubObjectName);
-          subObjects->CreateRootSignature(
-              rootSignatureSubObjectName, notALocalRS, extendedRootSig.data(),
-              static_cast<uint32_t>(extendedRootSig.size()));
-          break;
+          std::vector<uint8_t> ExtendedRootSignature =
+              AddUAVParamterToRootSignature(Data, Size, ToolsUAVRegister);
+          if (!ExtendedRootSignature.empty()) {
+            ReplacementRootSignatures.push_back(
+                {subObject.first.str(), std::move(ExtendedRootSignature)});
+          }
        }
      }
    }
+
+    constexpr bool NotALocalRootSignature = false;
+    for (const ReplacementRootSignature &Replacement :
+         ReplacementRootSignatures) {
+      subObjects->RemoveSubobject(Replacement.Name);
+      subObjects->CreateRootSignature(
+          Replacement.Name, NotALocalRootSignature, Replacement.Data.data(),
+          static_cast<uint32_t>(Replacement.Data.size()));


Damyan Pepper (damyanp) and others added 2 commits August 25, 2026 18:20

github-project-automation Bot added this to HLSL Roadmap Aug 27, 2026

github-project-automation Bot moved this to New in HLSL Roadmap Aug 27, 2026

Damyan Pepper (damyanp) requested a review from Austin Kinross (austinkinross) August 27, 2026 22:45

Austin Kinross (austinkinross) approved these changes Aug 27, 2026

View reviewed changes

Damyan Pepper (damyanp) marked this pull request as ready for review August 27, 2026 23:31

Copilot AI balanced review requested due to automatic review settings August 27, 2026 23:31

Copilot started reviewing on behalf of Damyan Pepper (damyanp) August 27, 2026 23:31 View session

Copilot AI reviewed Aug 27, 2026

View reviewed changes

Comment thread lib/DxilPIXPasses/PixPassHelpers.cpp Outdated

Damyan Pepper (damyanp) and others added 7 commits September 2, 2026 10:52

[PIX] Remove metadata counts from comments

fda0a59

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

[PIX] Format L1 validation changes

f80826f

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

Merge final approved clean L1 into original L2

8c15684

Bring the final approved L1 tip, including its formatting follow-up, into this layer while preserving the original L2 commit as the first parent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings September 3, 2026 02:13

Copilot started reviewing on behalf of Damyan Pepper (damyanp) September 3, 2026 02:13 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Comment thread lib/DxilPIXPasses/PixPassHelpers.cpp Outdated

Damyan Pepper (damyanp) and others added 5 commits September 3, 2026 09:32

[PIX] Name the root signature parameter

ad56207

Apply the explicitly approved naming exception to ExtendRootSig's existing parameter without changing any other pre-existing identifier. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings September 3, 2026 18:36

Copilot started reviewing on behalf of Damyan Pepper (damyanp) September 3, 2026 18:37 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Comment thread lib/DxilPIXPasses/DxilPIXMeshShaderOutputInstrumentation.cpp

Comment thread lib/DxilPIXPasses/PixPassHelpers.cpp

Damyan Pepper (damyanp) and others added 2 commits September 3, 2026 11:44

Merge final L1 metadata validation into L2

eb33d12

Bring the approved L1 metadata placement and payload validation forward without rewriting published L2. The merge is conflict-free and introduces no L2-specific logic. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings September 3, 2026 19:08

Copilot started reviewing on behalf of Damyan Pepper (damyanp) September 3, 2026 19:08 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Damyan Pepper (damyanp) and others added 3 commits September 17, 2026 11:45

[PIX] Remove unused mesh index declaration

947c113

Keep the materialized emitIndices function and erase it when a valid zero-output mesh shader has no index calls. Add a validating regression for that path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings September 17, 2026 20:50

Copilot started reviewing on behalf of Damyan Pepper (damyanp) September 17, 2026 20:50 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Damyan Pepper (damyanp) and others added 2 commits September 17, 2026 15:32

Merge upstream main into PIX validation L1

8aa777f

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 819bcd16-83fc-4619-ac1a-b12ee50a2fe8

Merge current-main L1 into published L2

cecb393

Bring the finalized L1 tip based on current main forward without rewriting published L2. The merge is conflict-free and adds no L2-specific logic. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings September 17, 2026 23:39

Copilot started reviewing on behalf of Damyan Pepper (damyanp) September 17, 2026 23:40 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Base automatically changed from users/damyanp/pix-fixes-01 to main September 22, 2026 00:23

Navigation Menu

[PIX] Fix shared instrumentation resource handling - #8842

[PIX] Fix shared instrumentation resource handling#8842
Damyan Pepper (damyanp) wants to merge 21 commits into
mainfrom
users/damyanp/pix-fixes-02

Damyan Pepper (damyanp) commented Aug 27, 2026 •

edited

Loading

Uh oh!

Damyan Pepper (damyanp) commented Aug 27, 2026

Uh oh!

azure-pipelines Bot commented Aug 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Damyan Pepper (damyanp) commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Damyan Pepper (damyanp) commented Aug 27, 2026

Uh oh!

azure-pipelines Bot commented Aug 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Suppressed comments (2)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Damyan Pepper (damyanp) commented Aug 27, 2026 •

edited

Loading