Skip to content

PIX shader debugger: Support dynamic indices for local arrays#7536

Merged
jeffnn merged 10 commits into
microsoft:mainfrom
jeffnn:PIX_DynamicLocalArrayIndices
Jun 18, 2025
Merged

PIX shader debugger: Support dynamic indices for local arrays#7536
jeffnn merged 10 commits into
microsoft:mainfrom
jeffnn:PIX_DynamicLocalArrayIndices

Conversation

@jeffnn
Copy link
Copy Markdown
Collaborator

@jeffnn jeffnn commented Jun 12, 2025

The root of the problem being addressed here is this line from the previous version of DxilAnnotateWithVirtualRegister.cpp at (old) line 251 in function GetStructOffset:

    auto *pArrayIndex =
        llvm::dyn_cast<llvm::ConstantInt>(pGEP->getOperand(GEPOperandIndex++));

When an array is dynamically indexed, this dyn_cast of course returns nullptr, and this function returns a zero, which eventually caused the values of all dynamically-indexed array elements in PIX's shader debugger to be reported as the value of the zeroth element in the array.

The next issue was that stores to an alloca-backed dynamic array weren't being properly recognized as significant events from PIX debugger's point of view. PIX adds its own "fake" alloca stores to help tie its debug output with the debug info that ends up in the PDB, so it's easy enough to co-opt that machinery to cover stores to "real" allocas, i.e. function-local array storage. To do so, the "AnnotateStore" function needs some of the metadata (i.e. PIX instruction number) that is added during runOnModule here. This necessitated rearranging runOnModule and putting stores into a vector that we then iterate over at the end of runOnModule.

Now that indices aren't collapsed into just the zeroth, PIX needs to know how much storage to allocate for the full array, which is the motivation for the change in DxilDebugInstrumentation.cpp to return some metadata that PIX can parse.

DxilDbgValueToDbgDeclare.cpp's changes are just a variable rename to aid readability.

The rearrangement of runOnModule can induce some allocas to be visited more than once, so there are changes in DxilPIXVirtualRegisters.cpp to make sure we don't overwrite an existing alloca ordinal with a new one (which would confuse previously-established references to that alloca).

file-check tests have been added to validate that
-the stores to local arrays are being noticed properly.
-the debug pass correctly outputs the metadata that informs PIX about alloca sizes

The majority of these changes really needs end-to-end testing in PIX, where I can gather real debug output as generated by the GPU in response to the instrumentation, then match those results up with PDB data and finally show HLSL variable contents in the shader debugger, so there are some tests waiting on the PIX side for when this change makes its way there.

Comment thread lib/DxilPIXPasses/DxilAnnotateWithVirtualRegister.cpp Outdated
Copy link
Copy Markdown
Contributor

@adam-yang adam-yang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just one minor comment.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 17, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Comment thread lib/DxilPIXPasses/DxilAnnotateWithVirtualRegister.cpp Outdated
@jeffnn jeffnn merged commit 8a77b0c into microsoft:main Jun 18, 2025
12 checks passed
@github-project-automation github-project-automation Bot moved this from New to Done in HLSL Roadmap Jun 18, 2025
jeffnn added a commit that referenced this pull request Jul 17, 2025
The root of the problem being addressed here is this line from the
previous version of DxilAnnotateWithVirtualRegister.cpp at (old) line
251 in function GetStructOffset:

```
    auto *pArrayIndex =
        llvm::dyn_cast<llvm::ConstantInt>(pGEP->getOperand(GEPOperandIndex++));
```

When an array is dynamically indexed, this dyn_cast of course returns
nullptr, and this function returns a zero, which eventually caused the
values of all dynamically-indexed array elements in PIX's shader
debugger to be reported as the value of the zeroth element in the array.

The next issue was that stores to an alloca-backed dynamic array weren't
being properly recognized as significant events from PIX debugger's
point of view. PIX adds its own "fake" alloca stores to help tie its
debug output with the debug info that ends up in the PDB, so it's easy
enough to co-opt that machinery to cover stores to "real" allocas, i.e.
function-local array storage. To do so, the "AnnotateStore" function
needs some of the metadata (i.e. PIX instruction number) that is added
during runOnModule here. This necessitated rearranging runOnModule and
putting stores into a vector that we then iterate over at the end of
runOnModule.

Now that indices aren't collapsed into just the zeroth, PIX needs to
know how much storage to allocate for the full array, which is the
motivation for the change in DxilDebugInstrumentation.cpp to return some
metadata that PIX can parse.

DxilDbgValueToDbgDeclare.cpp's changes are just a variable rename to aid
readability.

The rearrangement of runOnModule can induce some allocas to be visited
more than once, so there are changes in DxilPIXVirtualRegisters.cpp to
make sure we don't overwrite an existing alloca ordinal with a new one
(which would confuse previously-established references to that alloca).

file-check tests have been added to validate that
-the stores to local arrays are being noticed properly.
-the debug pass correctly outputs the metadata that informs PIX about
alloca sizes

The majority of these changes really needs end-to-end testing in PIX,
where I can gather real debug output as generated by the GPU in response
to the instrumentation, then match those results up with PDB data and
finally show HLSL variable contents in the shader debugger, so there are
some tests waiting on the PIX side for when this change makes its way
there.

(cherry picked from commit 8a77b0c)
jeffnn added a commit that referenced this pull request Jul 17, 2025
…7649)

cherry picked from commit
7e0d771
8a77b0c


Original PRs:

#7536
#7557

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants