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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl 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)
----------------
rnk wrote:
> 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.
Can you post you work-in-progress for the follow-up? I'd like to better understand where this is heading.


================
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);
----------------
rnk wrote:
> 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);
Still seems silly. I don't like that the implementation detail that "direct values are encoded with a second register operand" is leaking all over the place. Should we add a BuildMI variant that is useful in this situation? There's a bunch of other places that use the same pattern and I don't like how hard these are to find.

In any case we can do this later, too.


https://reviews.llvm.org/D37911