Skip to content

[PIX] [NFC] Test NURI shader flags - #8846

Merged
Damyan Pepper (damyanp) merged 1 commit into
mainfrom
users/damyanp/pix-fixes-05
Oct 6, 2026
Merged

[PIX] [NFC] Test NURI shader flags#8846
Damyan Pepper (damyanp) merged 1 commit into
mainfrom
users/damyanp/pix-fixes-05

Conversation

@damyanp

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

Copy link
Copy Markdown
Member

Add regression coverage for the NURI shader-flag validation path.

Exercise instrumentation on a dynamically indexed resource and verify that the resulting module contains WaveActiveAllEqual and passes validation. Also check that the validation helper distinguishes real diagnostics from boilerplate.

Assisted-by: Copilot

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

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

Recomputes DXIL shader flags after PIX NURI instrumentation inserts wave operations.

Changes:

  • Recollects shader flags before metadata re-emission.
  • Adds regression validation for the WaveOps flag.

Reviewed changes

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

File Description
lib/DxilPIXPasses/DxilNonUniformResourceIndexInstrumentation.cpp Recomputes shader flags after instrumentation.
tools/clang/unittests/HLSL/PixTest.cpp Tests instrumented module validation and wave-op insertion.

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

Comment thread tools/clang/unittests/HLSL/PixTest.cpp Outdated
Comment thread tools/clang/unittests/HLSL/PixTest.cpp
Copilot AI review requested due to automatic review settings September 29, 2026 02:06
Copilot AI reviewed Sep 29, 2026
View reviewed changes

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 address the invalid shader flags, with only minor style feedback remaining.

Review effort: Balanced
Findings: 1 Medium severity · 1 Low severity

Open (2)

Copilot AI review requested due to automatic review settings September 29, 2026 16:25
Copilot AI reviewed Sep 29, 2026
View reviewed changes

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

🔵 Needs a closer look

The stated shader-flag recomputation is unchanged context, so the intended functional fix is absent from this diff.

Review effort: Balanced
Findings: 1 Medium severity · 1 Low severity

Open (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Diff omits the stated shader flag recomputation change

lib/​DxilPIXPasses/​DxilNonUniformResourceIndexInstrumentation.cpp:164

The described behavior change is not present in this diff: CollectShaderFlagsForModule() on the following line is unchanged context, so this hunk only adds a comment around behavior already in the target. Please update/rebase the stack so the actual recomputation change is included in this PR (or revise the PR scope if the implementation belongs to an earlier stack entry); otherwise merging this PR cannot deliver the stated fix.

Copilot AI balanced review requested due to automatic review settings September 30, 2026 00:02
Copilot AI reviewed Sep 30, 2026
View reviewed changes

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

🔵 Needs a closer look

The new DXIL 1.6 test needs the established version guard to avoid failures with older validators.

Review effort: Balanced
Findings: 1 Medium severity · 1 Low severity

Open (2)

Base automatically changed from users/damyanp/pix-fixes-04 to main October 1, 2026 21:46
Copilot AI balanced review requested due to automatic review settings October 1, 2026 21:56
Copilot AI reviewed Oct 1, 2026
View reviewed changes

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

🔵 Needs a closer look

The new test unconditionally requires DXIL 1.6 and can fail with older supported compiler or validator configurations.

Review effort: Balanced
Findings: 1 Medium severity · 1 Low severity

Open (2)

Copilot AI balanced review requested due to automatic review settings October 1, 2026 22:57
Copilot AI reviewed Oct 1, 2026
View reviewed changes

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 correctly updates emitted flags and includes focused regression coverage.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)

@damyanp Damyan Pepper (damyanp) changed the title [PIX] Recompute shader flags after NURI instrumentation [NFC] Test NURI shader flags Oct 6, 2026
@damyanp
[NFC] Test NURI shader flags
0d18e0e 
Adds regression coverage that runs NURI instrumentation on a dynamically indexed resource and verifies the resulting module validates and contains WaveActiveAllEqual.

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 6, 2026 16:56
@damyanp Damyan Pepper (damyanp) changed the title [NFC] Test NURI shader flags [PIX] [NFC] Test NURI shader flags Oct 6, 2026
Copilot AI reviewed Oct 6, 2026
View reviewed changes

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 focused test-only changes correctly cover the intended validation paths without unresolved issues.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

@damyanp
Damyan Pepper (damyanp) merged commit 6487ff4 into main Oct 6, 2026
15 checks passed
@damyanp
Damyan Pepper (damyanp) deleted the users/damyanp/pix-fixes-05 branch October 6, 2026 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@austinkinross Austin Kinross (austinkinross) austinkinross approved these changes
@llvm-beanz Chris B (llvm-beanz) llvm-beanz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

HLSL Roadmap
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@damyanp @austinkinross @llvm-beanz