[PATCH] D37911: [DebugInfo] Insert DW_OP_deref when spilling indirect DBG_VALUEs

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked an inline comment as done.
rnk added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:500
   // Get or create the UserValue for (variable,offset).
-  bool IsIndirect = MI.isIndirectDebugValue();
+  bool IsIndirect = MI.getOperand(1).isImm();
   if (IsIndirect)
----------------
aprantl wrote:
> Why can't this be API?
I want to consider frame indices and constants as "indirect" so that they use the same UserValue. Some tests (I'll check how many) rely on this for eliminating adjacent DBG_VALUE instructions describing the same variable, like this:
  DBG_VALUE %vreg3, 0, !"v" # Goes away after rewriting if we use isImm() directly
  DBG_VALUE 42, 0, !"v"
  mov $42, %eax
  retq

In the long run, IsIndirect should not be considered when getting or creating a UserValue. I have a follow-on patch that implements this, but I split it out for incrementality.


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1058
   else
-    BuildMI(*MBB, I, getDebugLoc(), TII.get(TargetOpcode::DBG_VALUE))
-        .add(Loc)
-        .addImm(0U)
-        .addMetadata(Variable)
-        .addMetadata(Expression);
+    MIB.addReg(0U, RegState::Debug);
+  MIB.addMetadata(Variable).addMetadata(Expr);
----------------
aprantl wrote:
> There's a much easier to read BuildMI variant that takes an IsIndirect flag.
That one only takes a register argument, though. At this point we have a MachineOperand. I tried this first, but I couldn't set the debug-use flag:
  BuildMI(*MBB, I, getDebugLoc(), TII.get(TargetOpcode::DBG_VALUE))
      .add(Loc)
      .add(NewIndirect ? MachineOperand::CreateImm(0U)
                       : MachineOperand::CreateReg(0U))
      .addMetadata(Variable)
      .addMetadata(Expr);


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1071
     unsigned LocNo = I.value();
+    bool Spilled = LocNo != ~0U ? SpilledLocations.test(LocNo) : false;
 
----------------
aprantl wrote:
> shall we hide the comparison against ~0U in a self-duocumenting helper function isCondition(LocNo)?
I actually did that in the follow-up. Do you think it's worth it now?


https://reviews.llvm.org/D37911