Skip to content

Commit

Permalink
PIX: Fix recent regression in debug instrumentation of void instructi…
Browse files Browse the repository at this point in the history
…ons (microsoft#6053)

The misplacement of that "return" in DxilDebugInstrumentation.cpp meant
that a thread would continue to the following call to
addStepDebugEntryValue, even if pix_dxil::PixDxilReg::FromInst had
failed (i.e. returned false), which means that RegNum is not valid
(although initialized to 0).

This meant that PIX was instrumenting a bunch of void-return DXIL
instructions that it shouldn't have.
Didn't think to test that it WASN'T instrumenting instructions, but
herein is added a test to do just that.
  • Loading branch information
jeffnn authored Nov 22, 2023
1 parent 1b405f6 commit 71afbcc
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 5 deletions.
9 changes: 4 additions & 5 deletions lib/DxilPIXPasses/DxilDebugInstrumentation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -894,13 +894,12 @@ void DxilDebugInstrumentation::addStepDebugEntry(BuilderContext &BC,
}

std::uint32_t RegNum;
if (!pix_dxil::PixDxilReg::FromInst(Inst, &RegNum))
if (Inst->getOpcode() == Instruction::Ret) {
if (!pix_dxil::PixDxilReg::FromInst(Inst, &RegNum)) {
if (Inst->getOpcode() == Instruction::Ret)
addStepEntryForType<void>(DebugShaderModifierRecordTypeDXILStepTerminator,
BC, InstNum, nullptr, 0, 0);
return;
}

return;
}
addStepDebugEntryValue(BC, InstNum, Inst, RegNum, BC.Builder.getInt32(0));
}

Expand Down
17 changes: 17 additions & 0 deletions tools/clang/test/HLSLFileCheck/pix/DontDebugNoRegnum.hlsl
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// RUN: %dxc -Tlib_6_6 %s | %opt -S -dxil-annotate-with-virtual-regs -hlsl-dxil-debug-instrumentation | %FileCheck %s

// Check that the instrumentation does NOT instrument an instruction that has no dxil-inst-num metadata
// The load instruction should not be instrumented. If it is, we can expect an "atomicBinOp", emitted
// by the instrumentation, to be generated before the handle value is used, so assert that there
// is no such atomicBinOp:

// CHECK: [[HandleNum:%[0-9]+]] = load %dx.types.Handle,
// CHECK-NOT: call i32 @dx.op.atomicBinOp.i32(i32 78
// CHECK: @dx.op.createHandleForLib.dx.types.Handle(i32 160, %dx.types.Handle [[HandleNum]]

RWByteAddressBuffer buffer : register(u0);

[shader("raygeneration")]
void main() {
buffer.Store(0, 42);
}

0 comments on commit 71afbcc

Please sign in to comment.