[PIX] Instrument DebugBreak() calls for PIX#8349
[PIX] Instrument DebugBreak() calls for PIX#8349henchhinglimbu wants to merge 4 commits intomicrosoft:mainfrom
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@microsoft-github-policy-service agree company="Microsoft" |
| for (auto UI = DebugBreakFunc->use_begin(); | ||
| UI != DebugBreakFunc->use_end();) { | ||
| auto &Use = *UI++; | ||
| DebugBreakCalls.push_back(cast<CallInst>(Use.getUser())); | ||
| } |
There was a problem hiding this comment.
I have a few issues with this loop (excessive autos, not storing the "end" iterator, unnecessary *UI++ pattern when it's all const).
Would something like this work instead (untested suggestion)?
| for (auto UI = DebugBreakFunc->use_begin(); | |
| UI != DebugBreakFunc->use_end();) { | |
| auto &Use = *UI++; | |
| DebugBreakCalls.push_back(cast<CallInst>(Use.getUser())); | |
| } | |
| for (const Use &U: DebugBreakFunc->uses()) | |
| DebugBreakCalls.push_back(cast<CallInst>(U.getUser())); |
There was a problem hiding this comment.
Applied your suggestion.
| if (!PixUAVResource) { | ||
| PixUAVResource = | ||
| PIXPassHelpers::CreateGlobalUAVResource(DM, 0, "PixUAVResource"); | ||
| } |
There was a problem hiding this comment.
| if (!PixUAVResource) { | |
| PixUAVResource = | |
| PIXPassHelpers::CreateGlobalUAVResource(DM, 0, "PixUAVResource"); | |
| } | |
| if (!PixUAVResource) | |
| PixUAVResource = | |
| PIXPassHelpers::CreateGlobalUAVResource(DM, 0, "PixUAVResource"); |
LLVM coding conventions omit braces for single statement bodies.
|
|
||
| uint32_t InstructionNumber = 0; | ||
| if (!pix_dxil::PixDxilInstNum::FromInst(CI, &InstructionNumber)) { | ||
| DXASSERT_NOMSG(false); |
There was a problem hiding this comment.
What does it mean if this assert fires?
There was a problem hiding this comment.
It means that PIX instruction number metadata is missing or malformed. I can update the assert to include a message.
|
|
||
| // Remove the now-unused dx.op.debugBreak function declaration so the | ||
| // DebugBreak operation is fully eliminated from the module. | ||
| if (DebugBreakFunc->use_empty()) |
There was a problem hiding this comment.
If there are still uses of this, does this mean that something actually went wrong with our attempt to remove all uses? Is this something we'd want to know about via an assert or failure condition/
There was a problem hiding this comment.
This is more of a cleanup step. The DebugBreak calls have already been replaced with atomic UAV writes above, so the pass works correctly without this. We just remove the leftover declaration to keep the module clean. I can update the comment to better reflect this.
|
Azure Pipelines: 1 pipeline(s) require an authorized user to comment /azp run to run. |
|
Azure Pipelines: 1 pipeline(s) require an authorized user to comment /azp run to run. |
3 participants
Changes:
DebugBreak()calls with UAV atomic writes so PIX can detect hit locations without halting execution.Testing:
dxcompiler,ClangHLSLTests, andcheck-allcheck-allpassed locallyCo-authored-by: Copilot 223556219+Copilot@users.noreply.github.com