[PIX] Record only real resource accesses, including samplers by damyanp · Pull Request #8847 · microsoft/DirectXShaderCompiler

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

Do not record handle annotations or memory barriers as resource accesses. Continue tracing annotated handles so genuine accesses are still recorded.

Record sampler accesses in library shaders so sampler bindings are not missed.

Tests cover annotation and barrier handling, library sampler records, and dynamically indexed resources.

Assisted-by: Copilot

Copilot

Pull request overview

Corrects PIX shader access tracking to record genuine resource accesses, including library samplers, while excluding annotations and barriers.

Changes:

Adds sampler matching for library handles.
Excludes AnnotateHandle and resource barriers from access records.
Adds FileCheck and validation coverage.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`lib/DxilPIXPasses/DxilShaderAccessTracking.cpp`	Updates resource matching and non-access exclusions.
`tools/clang/unittests/HLSL/PixTest.cpp`	Extends pass configuration and validation tests.
`tools/clang/test/HLSLFileCheck/pix/AccessTrackingLibAnnotateHandleIsNotAnAccess.hlsl`	Tests annotation exclusion.
`tools/clang/test/HLSLFileCheck/pix/AccessTrackingLibAnnotatedHandleReadStillRecorded.hlsl`	Tests accesses through annotated handles.
`tools/clang/test/HLSLFileCheck/pix/AccessTrackingBarrierIsNotAnAccess.hlsl`	Tests barrier exclusion.

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

Copilot

Copilot review overview

🟢 Approval recommended

The focused behavior changes have targeted coverage; remaining feedback is non-blocking style cleanup.

Review effort: Balanced
Findings: 2 · 4

Open (6)

The default pass configuration declares only UAV ranges, but this shader accesses an SRV and a…
This helper does not actually constrain the match to the byte-offset operand: i32 264 in any…
Spell out the inferred types explicitly · New
This changes user-visible PIX access records and fixes sampler bindings being reported as unused,…
Please spell out this simple return type under the repository's almost-never-auto convention;…
Please use explicit types for these straightforward locals under the repository's…

Copilot

Copilot review overview

🟢 Approval recommended

The implementation is focused and adequately tested, with only minor style and spelling feedback outstanding.

Review effort: Balanced
Findings: 2 · 6

Open (8)

The default pass configuration declares only UAV ranges, but this shader accesses an SRV and a…
This helper does not actually constrain the match to the byte-offset operand: i32 264 in any…
Fix misspelling of delimiter · New
Use explicit std::string type instead of auto · New
Spell out the inferred types explicitly
This changes user-visible PIX access records and fixes sampler bindings being reported as unused,…
Please spell out this simple return type under the repository's almost-never-auto convention;…
Please use explicit types for these straightforward locals under the repository's…

Copilot

Copilot review overview

🔵 Needs a closer look

The sampler regression assertion can falsely accept an incorrect byte offset, and new declarations violate the explicit-type convention.

Review effort: Balanced
Findings: 2 · 6

Open (8)

The default pass configuration declares only UAV ranges, but this shader accesses an SRV and a…
This helper does not actually constrain the match to the byte-offset operand: i32 264 in any…
Use explicit std::string type instead of auto
Fix misspelling of delimiter
Spell out the inferred types explicitly
This changes user-visible PIX access records and fixes sampler bindings being reported as unused,…
Please spell out this simple return type under the repository's almost-never-auto convention;…
Please use explicit types for these straightforward locals under the repository's…

Copilot

Copilot review overview

🟢 Approval recommended

The implementation is focused and well tested; the remaining findings are minor explicit-type convention issues.

Review effort: Balanced
Findings: 2 · 6

Open (8)

The default pass configuration declares only UAV ranges, but this shader accesses an SRV and a…
This helper does not actually constrain the match to the byte-offset operand: i32 264 in any…
Use explicit std::string type instead of auto
Fix misspelling of delimiter
Spell out the inferred types explicitly
This changes user-visible PIX access records and fixes sampler bindings being reported as unused,…
Please spell out this simple return type under the repository's almost-never-auto convention;…
Please use explicit types for these straightforward locals under the repository's…

Copilot

Copilot review overview

🟢 Approval recommended

The behavioral changes have focused regression coverage; remaining feedback concerns only explicit-type conventions.

Review effort: Balanced
Findings: 2 · 6

Open (8)

The default pass configuration declares only UAV ranges, but this shader accesses an SRV and a…
This helper does not actually constrain the match to the byte-offset operand: i32 264 in any…
Use explicit std::string type instead of auto
Fix misspelling of delimiter
Spell out the inferred types explicitly
This changes user-visible PIX access records and fixes sampler bindings being reported as unused,…
Please spell out this simple return type under the repository's almost-never-auto convention;…
Please use explicit types for these straightforward locals under the repository's…

Exclude annotateHandle and barrierByMemoryHandle from access records while still tracing annotated handles for real accesses. Match library sampler handles so sampler accesses are recorded, with regression coverage for these paths. Co-authored-by: Copilot App <[email protected]> Copilot-Session: 40dc9de3-617e-4caf-ab0d-fba0a033ed93

Copilot

🔵 Needs a closer look

The user-visible PIX behavior fix needs release-note coverage or confirmation of planned shared coverage.

0 open findings

8 resolved since last review

The default pass configuration declares only UAV ranges, but this shader accesses an SRV and a…
This helper does not actually constrain the match to the byte-offset operand: i32 264 in any…
Use explicit std::string type instead of auto
Fix misspelling of delimiter
Spell out the inferred types explicitly
This changes user-visible PIX access records and fixes sampler bindings being reported as unused,…
Please spell out this simple return type under the repository's almost-never-auto convention;…
Please use explicit types for these straightforward locals under the repository's…

🧠 Review effort: Balanced

Finn Plummer (inbelic) · 2026-10-08T22:19:56Z

+// ResourceAccessStyle in the next four. RayGeneration is 7 and SRVRead is
+// 5, so 0x75000000 == 1962934272.
+
+// CHECK-NOT: bufferStore.i32(i32 69, %dx.types.Handle {{.*}}, i32 16,


nit: this will technically not catch the case if this operation is between any of the following 3 ops. I think there is a filecheck flag like -implicit-check-not that would check not everywhere. Not sure if it is in llvm 3.7 filecheck tho

GitHub Copilot agent: Implemented in dc2ac17. The Windows FileCheck test wrapper rejects --implicit-check-not, so the test now repeats the offset-16 CHECK-NOT between every positive check, including before the first and after the last. The standard PIX FileCheck suite passes (71/71).

Deric C. (Icohedron) · 2026-10-08T22:30:21Z

+  for (const std::string &line : instrumentedLines) {
+    if (line.find("dx.op.bufferStore") != std::string::npos) {
+      hasBufferStore = true;
+      break;
+    }
+  }
+  VERIFY_IS_TRUE(hasBufferStore);


Check the indexed texture's store, not any bufferStore?

Suggested change

for (const std::string &line : instrumentedLines) {

if (line.find("dx.op.bufferStore") != std::string::npos) {

hasBufferStore = true;

break;

}

}

VERIFY_IS_TRUE(hasBufferStore);

const bool trackedIndexedTexture = std::any_of(

instrumentedLines.begin(), instrumentedLines.end(),

[](const std::string &line) {

return line.find("call void @dx.op.bufferStore.i32(") !=

std::string::npos &&

line.find(", i32 %OffsetByteIndex,") != std::string::npos;

});

VERIFY_IS_TRUE(trackedIndexedTexture);

GitHub Copilot agent: Implemented in dc2ac17. The test now requires a bufferStore with i32 %OffsetByteIndex after the handle, rather than accepting any bufferStore. The PixTest suite passes (73/73).

Finn Plummer (inbelic)

LGTUS Alex Sepkowski (@alsepkow) Deric C. (@Icohedron) - some nits inline

Check forbidden resource writes across every FileCheck interval and require the dynamic texture tracking store to use its computed byte offset. Co-authored-by: Copilot App <[email protected]> Copilot-Session: 40dc9de3-617e-4caf-ab0d-fba0a033ed93

Copilot

🟢 Approval recommended

The focused implementation is consistent with the pass architecture and has appropriate regression coverage.

0 open findings

🧠 Review effort: 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 23:37

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

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

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

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

View reviewed changes

Copilot AI reviewed Aug 27, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 29, 2026 02:24

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

Copilot AI reviewed Sep 29, 2026

View reviewed changes

Comment thread tools/clang/unittests/HLSL/PixTest.cpp Outdated

Copilot AI review requested due to automatic review settings September 29, 2026 16:26

Copilot started reviewing on behalf of Damyan Pepper (damyanp) September 29, 2026 16:27 View session

Copilot AI reviewed Sep 29, 2026

View reviewed changes

Comment thread tools/clang/unittests/HLSL/PixTest.cpp Outdated

Comment thread tools/clang/unittests/HLSL/PixTest.cpp Outdated

Copilot AI balanced review requested due to automatic review settings September 30, 2026 00:03

Copilot started reviewing on behalf of Damyan Pepper (damyanp) September 30, 2026 00:04 View session

Copilot AI reviewed Sep 30, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 1, 2026 21:58

Damyan Pepper (damyanp) force-pushed the users/damyanp/pix-fixes-06 branch from 80a9c34 to 79770f0 Compare October 1, 2026 21:58

Copilot started reviewing on behalf of Damyan Pepper (damyanp) October 1, 2026 21:58 View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 1, 2026 23:10

Damyan Pepper (damyanp) force-pushed the users/damyanp/pix-fixes-06 branch from 79770f0 to 810a923 Compare October 1, 2026 23:10

Copilot started reviewing on behalf of Damyan Pepper (damyanp) October 1, 2026 23:11 View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Base automatically changed from users/damyanp/pix-fixes-05 to main October 6, 2026 21:00

Damyan Pepper (damyanp) force-pushed the users/damyanp/pix-fixes-06 branch from 810a923 to fa0c17b Compare October 6, 2026 21:00

Copilot AI balanced review requested due to automatic review settings October 7, 2026 18:03

Damyan Pepper (damyanp) force-pushed the users/damyanp/pix-fixes-06 branch from fa0c17b to 6d4c693 Compare October 7, 2026 18:03

Copilot started reviewing on behalf of Damyan Pepper (damyanp) October 7, 2026 18:03 View session

Copilot AI reviewed Oct 7, 2026

View reviewed changes

Finn Plummer (inbelic) reviewed Oct 8, 2026

View reviewed changes

Deric C. (Icohedron) reviewed Oct 8, 2026

View reviewed changes

Finn Plummer (inbelic) reviewed Oct 8, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 9, 2026 03:22

Copilot started reviewing on behalf of Damyan Pepper (damyanp) October 9, 2026 03:22 View session

Copilot AI reviewed Oct 9, 2026

View reviewed changes

-  for (const std::string &line : instrumentedLines) {
-    if (line.find("dx.op.bufferStore") != std::string::npos) {
-      hasBufferStore = true;
-      break;
-    }
-  }
-  VERIFY_IS_TRUE(hasBufferStore);
+  const bool trackedIndexedTexture = std::any_of(
+      instrumentedLines.begin(), instrumentedLines.end(),
+      [](const std::string &line) {
+        return line.find("call void @dx.op.bufferStore.i32(") !=
+                   std::string::npos &&
+               line.find(", i32 %OffsetByteIndex,") != std::string::npos;
+      });
+  VERIFY_IS_TRUE(trackedIndexedTexture);

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

Finn Plummer (inbelic) Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Damyan Pepper (damyanp) Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

Deric C. (Icohedron) Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

Damyan Pepper (damyanp) Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

Finn Plummer (inbelic) left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Damyan Pepper (damyanp) commented Aug 27, 2026 •

edited

Loading

Finn Plummer (inbelic) Oct 8, 2026 •

edited

Loading