-
Notifications
You must be signed in to change notification settings - Fork 781
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
Conversation
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
adam-yang
reviewed
Jun 16, 2025
adam-yang
approved these changes
Jun 16, 2025
There was a problem hiding this 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.
✅ With the latest revision this PR passed the C/C++ code formatter. |
adam-yang
reviewed
Jun 17, 2025
adam-yang
approved these changes
Jun 17, 2025
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.
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:
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.