[PIX] Fix pixel-hit signature instrumentation - #8852
Open
Damyan Pepper (damyanp) wants to merge 1 commit into
Open
[PIX] Fix pixel-hit signature instrumentation#8852Damyan Pepper (damyanp) wants to merge 1 commit into
Damyan Pepper (damyanp) wants to merge 1 commit into
Conversation
PIX must know which signature row carries SV_Position, so that it can match a pixel with its instrumentation record. The pass has one option for this row and no way to state whether the row is a requirement or a guess. An unverified row can evict a real interpolant. Moving SV_Position makes the input signature larger, so the pass must repack the displaced elements. A repack that cannot complete leaves the signature partly rewritten. Signature growth also invalidates the ViewID state, which the pass keeps. The pass scales the pixel-counter element offset to a byte offset and then limits it. An element index large enough to overflow the multiplication wraps before the limit applies, so an out-of-range pixel is attributed to byte 0, which is pixel 0. A render-target width or pixel count that is zero, negative, or too large for 32-bit offset arithmetic produces a shader that wraps silently. Two options carry the caller's intent. preferred-sv-position-row is a hint, and the pass uses the row only when the row is free. required-sv-position-row is a requirement, and the pass fails when it cannot honour the row, because a silent relocation would make PIX's correlation wrong without the caller knowing. The option upstream-sv-position-row stays accepted, because PIX builds older than the rename still send it. When a caller sends both hint spellings, preferred-sv-position-row wins. The camelCase options of the debug instrumentation pass do not change. The pass raises E_FAIL for a width or a pixel count it cannot represent. A silent wrap gives the caller wrong data with no indication. Assisted-by: Copilot Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 40dc9de3-617e-4caf-ab0d-fba0a033ed93
Damyan Pepper (damyanp)
requested a review
from Austin Kinross (austinkinross)
August 27, 2026 23:56
Damyan Pepper (damyanp)
marked this pull request as ready for review
August 27, 2026 23:57
Austin Kinross (austinkinross)
approved these changes
Aug 27, 2026
| if (RequiredRow != PIXPassHelpers::kUnknownSVPositionRow) { | ||
| m_upstreamSVPositionRow = RequiredRow; | ||
| m_svPositionRowAuthority = | ||
| PIXPassHelpers::SVPositionRowAuthority::Authoritative; |
There was a problem hiding this comment.
looks like copilot changed the command line param names but not the code variables?
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes PIX pixel-hit and debug instrumentation to safely handle SV_Position placement, signature growth, counter arithmetic, and saturated vertex signatures.
Changes:
- Adds hint versus authoritative
SV_Positionrow options with legacy compatibility. - Repackages displaced signature elements and invalidates stale ViewID state.
- Adds dimension validation, safer counter indexing, and extensive PIX tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
utils/hct/hctdb.py |
Registers new and legacy row options. |
tools/clang/unittests/HLSL/PixTest.cpp |
Adds instrumentation and validation tests. |
tools/clang/test/HLSLFileCheck/pix/pixelCounterRelocationRepacksIntoSharedRow.hlsl |
Tests compact repacking. |
tools/clang/test/HLSLFileCheck/pix/pixelCounterRelocationAtSignatureLimit.hlsl |
Tests relocation at the register limit. |
tools/clang/test/HLSLFileCheck/pix/pixelCounterPreferredRowWinsOverLegacyRow.hlsl |
Tests option precedence. |
tools/clang/test/HLSLFileCheck/pix/pixelCounterPreferredRowOptionIsAHint.hlsl |
Tests preferred-row hint behavior. |
tools/clang/test/HLSLFileCheck/pix/pixelCounterLegacyRowOptionIsAHint.hlsl |
Tests legacy hint behavior. |
tools/clang/test/HLSLFileCheck/pix/MeshShaderSignatureIsNotRelocated.hlsl |
Covers mesh-signature preservation. |
tools/clang/test/HLSLFileCheck/pix/GeometryShaderMultiStreamSignatureIsNotRelocated.hlsl |
Covers multi-stream GS preservation. |
tools/clang/test/HLSLFileCheck/pix/DebugVSParameters.hlsl |
Updates VS interpolation expectations. |
tools/clang/test/HLSLFileCheck/pix/DebugVertexShaderInputSignatureFull.hlsl |
Tests fully occupied VS signatures. |
tools/clang/test/HLSLFileCheck/pix/DebugEmitCorrectViewIdStatePS.hlsl |
Updates ViewID state expectations. |
tools/clang/test/HLSLFileCheck/pix/DebugDenseVertexShaderInput.hlsl |
Tests partially available VS identity. |
tools/clang/test/HLSLFileCheck/pix/DebugBasic.hlsl |
Updates SV_Position interpolation metadata. |
tools/clang/test/HLSLFileCheck/pix/DebugAuthoritativeSVPositionRow.hlsl |
Tests authoritative debug placement. |
lib/DxilPIXPasses/PixPassHelpers.h |
Defines shared row authority and ViewID helpers. |
lib/DxilPIXPasses/PixPassHelpers.cpp |
Implements placement, repacking, rollback, and state clearing. |
lib/DxilPIXPasses/DxilDebugInstrumentation.cpp |
Handles row authority and saturated VS signatures. |
lib/DxilPIXPasses/DxilAddPixelHitInstrumentation.cpp |
Updates validation, return scanning, and offset arithmetic. |
Suppressed comments (1)
lib/DxilPIXPasses/DxilAddPixelHitInstrumentation.cpp:213
- The new clamp still occurs after two unchecked 32-bit operations. For accepted inputs
RTWidth = 0x40000000,X = 0, andY = 4,Y * RTWidthwraps to zero beforeUMin, so this out-of-range pixel is still attributed to pixel 0. Compute the linear index with overflow-safe/saturating arithmetic (or in a wider type) before clamping and truncating.
auto YOffset = Builder.CreateMul(YAsInt, RTWidthArg, "YOffset");
auto Elementoffset =
Builder.CreateAdd(XAsInt, YOffset, "ElementOffset");
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
55
to
56
| GetPassOptionInt(O, "rt-width", &RTWidth, 0); | ||
| GetPassOptionInt(O, "num-pixels", &NumPixels, 0); |
Comment on lines
+692
to
693
| if (Existing_SV_Position != InputElements.end()) | ||
| return Existing_SV_Position->get()->GetID(); |
Comment on lines
+715
to
+725
| unsigned int TargetRow = kUnknownSVPositionRow; | ||
| if (UpStreamSVPosRow < hlsl::DXIL::kMaxSignatureTotalVectors) { | ||
| bool const RowIsOccupied = | ||
| !FindElementsOccupyingSignatureRow(InputElements, UpStreamSVPosRow) | ||
| .empty(); | ||
| bool const MayDisplaceOccupants = | ||
| RowAuthority == SVPositionRowAuthority::Authoritative && | ||
| ShaderKind == hlsl::DXIL::ShaderKind::Pixel; | ||
| if (!RowIsOccupied || MayDisplaceOccupants) | ||
| TargetRow = UpStreamSVPosRow; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PIX must know which signature row carries SV_Position, so that it can match a pixel with its instrumentation record. The pass has one option for this row and no way to state whether the row is a requirement or a guess. An unverified row can evict a real interpolant.
Moving SV_Position makes the input signature larger, so the pass must repack the displaced elements. A repack that cannot complete leaves the signature partly rewritten. Signature growth also invalidates the ViewID state, which the pass keeps.
The pass scales the pixel-counter element offset to a byte offset and then limits it. An element index large enough to overflow the multiplication wraps before the limit applies, so an out-of-range pixel is attributed to byte 0, which is pixel 0.
A render-target width or pixel count that is zero, negative, or too large for 32-bit offset arithmetic produces a shader that wraps silently.
Two options carry the caller's intent. preferred-sv-position-row is a hint, and the pass uses the row only when the row is free. required-sv-position-row is a requirement, and the pass fails when it cannot honour the row, because a silent relocation would make PIX's correlation wrong without the caller knowing. The option upstream-sv-position-row stays accepted, because PIX builds older than the rename still send it. When a caller sends both hint spellings, preferred-sv-position-row wins. The camelCase options of the debug instrumentation pass do not change.
The pass raises E_FAIL for a width or a pixel count it cannot represent. A silent wrap gives the caller wrong data with no indication.
Assisted-by: Copilot
Stack created with GitHub Stacks CLI • Give Feedback 💬