[PIX] Record only real resource accesses, including samplers - #8847
Damyan Pepper (damyanp) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
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
AnnotateHandleand 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.
| std::string needle = "i32 " + std::to_string(byteOffset); | ||
| for (auto const &line : lines) { | ||
| if (line.find("dx.op.bufferStore") != std::string::npos && | ||
| line.find(needle) != std::string::npos) { | ||
| return true; | ||
| } | ||
| } |
| auto compiled = Compile(m_dllSupport, hlsl, L"lib_6_6", {L"-Od"}); | ||
| auto output = RunShaderAccessTrackingPass( | ||
| compiled, L"S0:0:4i0;M0:20:4i0;U0:40:4i0;.0;0;0."); | ||
| auto lines = Split(Disassemble(output.blob), '\n'); |
| })x"; | ||
|
|
||
| auto compiled = Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); | ||
| auto output = RunShaderAccessTrackingPass(compiled); |
| return textures[index].Sample(samp, pos.xy); | ||
| })x"; | ||
|
|
||
| auto compiled = Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); |
| if (global == Sampler->GetGlobalSymbol()) { | ||
| binding = | ||
| hlsl::resource_helper::loadBindingFromResourceBase(Sampler.get()); | ||
| ret.registerType = RegisterType::Sampler; |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused behavior changes have targeted coverage; remaining feedback is non-blocking style cleanup.
Review effort: Balanced
Findings: 2
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 264in 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-autoconvention;… Please use explicit types for these straightforward locals under the repository's…
| auto compiled = Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); | ||
| auto output = RunShaderAccessTrackingPass(compiled); |
There was a problem hiding this comment.
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
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 264in 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-autoconvention;… Please use explicit types for these straightforward locals under the repository's…
| ValidateAccessTrackingMods(hlsl, true); | ||
| } | ||
|
|
||
| std::vector<std::string> Split(std::string str, char delimeter); |
| static bool HasBufferStoreWithByteOffset(std::vector<std::string> const &lines, | ||
| unsigned byteOffset) { | ||
| std::string needle = "i32 " + std::to_string(byteOffset); | ||
| for (auto const &line : lines) { |
There was a problem hiding this comment.
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
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 264in 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-autoconvention;… Please use explicit types for these straightforward locals under the repository's…
80a9c34 to
79770f0
Compare
There was a problem hiding this comment.
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
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 264in 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-autoconvention;… Please use explicit types for these straightforward locals under the repository's…
Shader access tracking no longer records annotateHandle or barrierByMemoryHandle as accesses, still looks through annotated handles to the resource, and matches library handles against samplers so sampler accesses are recorded. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 40dc9de3-617e-4caf-ab0d-fba0a033ed93
79770f0 to
810a923
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The behavioral changes have focused regression coverage; remaining feedback concerns only explicit-type conventions.
Review effort: Balanced
Findings: 2
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 264in 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-autoconvention;… Please use explicit types for these straightforward locals under the repository's…


The shader access tracking pass records an access for each DXIL operation that takes a resource handle. Two of these operations are not accesses. annotateHandle attaches type information to a handle. barrierByMemoryHandle puts accesses in order. The pass therefore reports a read or a write at a point where the shader touches no memory.
The pass also matches a library handle against the UAVs, the SRVs, and the constant buffers, but not against the samplers. A sampler access in a library shader gets no record, so the sampler binding looks unused.
An access that consumes an annotated handle keeps its record. The pass still looks through the annotation to reach the resource.
PIX receives fewer incorrect records and more sampler records. A tool that counts records will see different totals.
Assisted-by: Copilot
Stack created with GitHub Stacks CLI • Give Feedback 💬