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

[Reid Kleckner via Phabricator via llvm-commits]

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:
> 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.
D37932. It needs tests, though. I haven't found a way to write any without waiting for llvm.dbg.addr or manually editting MIR, so I held off on it. But, the basic idea is that a DBG_VALUE location is more than just one MachineOperand: it's that plus the IsIndirect bit.


================
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:
> 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.
There's a lot we can do to clean up DBG_VALUE construction and spilling. I'd like to handle these cases, get some tests in, and then come back and revisit the representation and API to build DBG_VALUEs. One idea is to have a separate DBG_ADDR instruction mirroring the llvm.dbg.addr intrinsic.


https://reviews.llvm.org/D37911