[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 💬}

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 <[email protected]> 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.

+    ExtendRootSig<DxilRootSignatureDesc1, DxilRootParameter1>(rs->Desc_1_1,
+                                                              toolsUAVRegister);
    rs->Desc_1_1.pParameters[rs->Desc_1_1.NumParameters - 1].Descriptor.Flags =
        hlsl::DxilRootDescriptorFlags::None;


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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

+void EraseIfUnused(hlsl::DxilModule &DM, llvm::Function *OpFunction) {
+  if (OpFunction != nullptr && OpFunction->user_empty()) {
+    DM.GetOP()->RemoveFunction(OpFunction);
+    OpFunction->eraseFromParent();
+  }


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 <[email protected]>

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 <[email protected]>

Apply the explicitly approved naming exception to ExtendRootSig's existing parameter without changing any other pre-existing identifier. Co-authored-by: Copilot App <[email protected]>

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

+  for (Function *StoreVertexOutputFunction : StoreVertexOutputFunctions) {
+    PIXPassHelpers::eraseIfUnused(DM, StoreVertexOutputFunction);
+  }



  auto *ret = pUAV.get();
  DM.AddUAV(std::move(pUAV));
+  DM.CollectShaderFlagsForModule();


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 <[email protected]>

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

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

Comment on lines 246 to 249

ExtendRootSig<DxilRootSignatureDesc1, DxilRootParameter1>(rs->Desc_1_1,

toolsUAVRegister);

rs->Desc_1_1.pParameters[rs->Desc_1_1.NumParameters - 1].Descriptor.Flags =

hlsl::DxilRootDescriptorFlags::None;

Damyan Pepper (damyanp) and others added 3 commits September 2, 2026 19:03

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 <[email protected]>

[NFC] Spell out L2 local types

27008ea

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 <[email protected]>

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

Comment on lines +389 to +393

void EraseIfUnused(hlsl::DxilModule &DM, llvm::Function *OpFunction) {

if (OpFunction != nullptr && OpFunction->user_empty()) {

DM.GetOP()->RemoveFunction(OpFunction);

OpFunction->eraseFromParent();

}

Damyan Pepper (damyanp) and others added 3 commits September 3, 2026 10:35

Merge final L1 naming tip into published L2

eae636f

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 <[email protected]>

[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 <[email protected]>

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

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 <[email protected]>

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

Navigation Menu

Uh oh!

[PIX] Fix shared instrumentation resource handling - #8842

[PIX] Fix shared instrumentation resource handling#8842
Damyan Pepper (damyanp) wants to merge 8 commits into
users/damyanp/pix-fixes-01from
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!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

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!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Damyan Pepper (damyanp) commented Aug 27, 2026 •

edited

Loading