Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jun 18, 2025

Conversation

jeffnn
Copy link
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.

Copy link
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.

Copy link
Contributor

github-actions bot commented Jun 17, 2025

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

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants