[PATCH] D88898: [DebugInfo][InstrRef][4/4] Support using DBG_INSTR_REF through all* backend passes

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

Hi,

Full disclosure: I've not looked at the instruction-referencing work for a little while and I don't remember all the details of the full patch-set. That said, I think this patch LGTM by itself.

I've left some comments inline and there is an inline comment from @djtodoro. Does anyone else have any comments (and should this patch have some reviewers assigned to it)? If not I will tentatively accept this soon.

Thanks,
Orlando



================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1634-1635
+
+    // Start block index: find the first non-debug instr in the block, and
+    // insert in front of it.
+    if (Idx == Slots->getMBBStartIdx(P.MBB)) {
----------------
nit: maybe it's just me but "insert in front of it" feels a bit ambiguous in this scenario, perhaps "insert before it" is clearer?


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1644
+    MachineInstr *Pos = Slots->getInstructionFromIndex(Idx);
+    if (Pos) {
+      // Insert at the end of any debug instructions.
----------------
nit: putting the declaration in the `if` feels better to me, but it doesn't really matter much.
```
if (MachineInstr *Pos = Slots->getInstructionFromIndex(Idx)) {
```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5581
+  if (Op->isReg())
+    NewMI = MakeVRegDbgValue(Op->getReg(), Expr, IsDbgDeclare);
+  else
----------------
Shouldn't the `MakeVRegDbgValue` parameter `Indirect` here be `IsIndirect` rather than `IsDbgDeclare`, or are `IsIndirect` and  `IsDbgDeclare` equivalent at this point? I'm just looking at the removed code in the diff so could be missing something.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:4080
+    // Mutating this instruction invalidates any debug data associated with it.
+    CmpInstr.setDebugInstrNum(0);
     // Fall through to optimize Cmp if Cmp is CMPrr or CMPri.
----------------
Is this covered by any of the tests?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88898/new/

https://reviews.llvm.org/D88898