Skip to content

[PIX] Fix dynamic bind-point identity and range bounds - #8848

Open
Damyan Pepper (damyanp) wants to merge 1 commit into
users/damyanp/pix-fixes-06from
users/damyanp/pix-fixes-07
Open

Damyan Pepper (damyanp) wants to merge 1 commit into
users/damyanp/pix-fixes-06from
users/damyanp/pix-fixes-07

Conversation

@damyanp

@damyanp Damyan Pepper (damyanp) commented Aug 27, 2026 •

Copy link
Copy Markdown
Member

Part 7 of 14 in the PIX instrumentation stack. It targets users/damyanp/pix-fixes-06, which supplies the access-record test support. Both changes are in the same pass.

RSRegisterIdentifier::operator< combines its three fields with &&. This is not a strict weak ordering. Two distinct ranges can compare as neither less than the other, so the map treats them as one key and gives their slot assignments to one of them. Accesses to one range are then reported against the other.

RegisterID comes from the position of the resource in the module's resource list. PIX needs the register that the binding occupies.

The range check for a constant index is off by one, so the pass accepts an index that addresses the slot after the range.

An index outside the range is still recorded at slot zero.

The meaning of RegisterID changes. A tool that resolved the old value against the module's resource list must resolve the new value against the root signature.

Assisted-by: Copilot

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


Stack created with GitHub Stacks CLI • Give Feedback 💬

@damyanp
Damyan Pepper (damyanp) marked this pull request as ready for review August 27, 2026 23:40
Copilot AI balanced review requested due to automatic review settings August 27, 2026 23:40

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.

Pull request overview

Fixes PIX resource access tracking so dynamic bind points use root-signature identities and range boundaries are enforced correctly.

Changes:

  • Implements strict bind-point ordering and correct register identities.
  • Fixes the constant-index upper-bound check.
  • Adds regression tests for both behaviors.

Reviewed changes

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

File Description
lib/DxilPIXPasses/DxilShaderAccessTracking.cpp Corrects ordering, bounds checking, and register identity.
tools/clang/unittests/HLSL/PixTest.cpp Adds PIX access-tracking regression tests.
Suppressed comments (3)

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

  • These inferred types are all short (CComPtr<IDxcBlob>, PassOutput, and std::string), so using auto conflicts with the repository's “almost never auto” convention.
  auto compiled = Compile(m_dllSupport, hlsl, L"cs_6_0", {L"-Od"}, L"CSMain");
  auto output =
      RunShaderAccessTrackingPass(compiled, L"S0:0:2i0;U0:0:10i0;.0;0;0.");
  auto text = JoinLines(output.lines);

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

  • The inferred types here are short and unambiguous, so auto conflicts with the repository's “almost never auto” convention. Please make all three declarations explicit.
  auto compiled = Compile(m_dllSupport, hlsl, L"cs_6_0", {L"-Od"}, L"CSMain");
  auto output = RunShaderAccessTrackingPass(compiled, L"U0:0:1i0;.0;0;0.");
  auto lines = Split(Disassemble(output.blob), '\n');

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

  • These inferred types are short and already exposed by the helper APIs. Please use explicit CComPtr<IDxcBlob>, PassOutput, and std::string types to follow the repository's “almost never auto” convention.
  auto compiled = Compile(m_dllSupport, hlsl, L"cs_6_6", {L"-Od"}, L"CSMain");
  auto output = RunShaderAccessTrackingPass(compiled, L"U0:0:10i0;.0;0;0.");
  auto text = JoinLines(output.lines);

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


static std::string JoinLines(std::vector<std::string> const &lines) {
std::string joined;
for (auto const &line : lines) {
Copilot AI review requested due to automatic review settings September 29, 2026 02:40

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.

Copilot review overview

🟢 Approval recommended

The implementation and regression coverage are sound; remaining feedback is style-only.

Review effort: Balanced
Findings: 2 Low severity

Open (2)

Comment on lines +1455 to +1458
auto compiled = Compile(m_dllSupport, hlsl, L"cs_6_0", {L"-Od"}, L"CSMain");
auto output =
RunShaderAccessTrackingPass(compiled, L"S0:0:2i0;U0:0:10i0;.0;0;0.");
auto text = JoinLines(output.lines);
Copilot AI review requested due to automatic review settings September 29, 2026 16:28

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.

Copilot review overview

🟢 Approval recommended

The functional fixes are correct and adequately covered; remaining feedback concerns only non-blocking type-style conventions.

Review effort: Balanced
Findings: 2 Low severity

Open (2)

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

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.

Copilot review overview

🟢 Approval recommended

The functional fixes are sound and appropriately tested; remaining feedback is limited to non-blocking type-style consistency.

Review effort: Balanced
Findings: 2 Low severity

Open (2)

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

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.

Copilot review overview

🟢 Approval recommended

The functional changes are correct and well tested; remaining feedback concerns minor explicit-type conventions.

Review effort: Balanced
Findings: 2 Low severity

Open (2)

Make RSRegisterIdentifier a strict weak ordering, report the register a binding occupies rather than its resource-list position, and fix the off-by-one range check for constant indices.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 40dc9de3-617e-4caf-ab0d-fba0a033ed93
Copilot AI balanced review requested due to automatic review settings October 1, 2026 23:10

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.

Copilot review overview

🟡 Changes recommended

Release-note coverage and explicit-type convention issues remain unresolved.

Review effort: Balanced
Findings: 4 Low severity

Open (4)

Comment on lines +751 to +752
// RegisterID is the binding lower bound, not the resource-list ID.
ret.RegisterID = resource->GetLowerBound();
Comment on lines +1480 to +1482
auto compiled = Compile(m_dllSupport, hlsl, L"cs_6_6", {L"-Od"}, L"CSMain");
auto output = RunShaderAccessTrackingPass(compiled, L"U0:0:10i0;.0;0;0.");
auto text = JoinLines(output.lines);

This branch has not been deployed

No deployments
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.

3 participants