[DebugInfo] Add missing DW_OP_deref when an NRVO pointer is spilled

[rnk]

Summary

Fixes PR34513.

Indirect DBG_VALUEs typically come from dbg.declares of non-trivially
copyable C++ objects that must be passed by address. We were already
handling the case where the virtual register gets allocated to a
physical register and is later spilled. That's what usually happens for
normal parameters that aren't NRVO variables: they usually appear in
physical register parameters, and are spilled later in the function,
which would correctly add deref.

NRVO variables are different because the dbg.declare can come much later
after earlier instructions cause the incoming virtual register to be
spilled.

Also, clean up this code. We only need to look at the first operand of a
DBG_VALUE, which eliminates the operand loop.

[aprantl]

llvm/lib/CodeGen/RegAllocFast.cpp

894 ↗

(On Diff #115480)

Nice.

llvm/test/DebugInfo/X86/dbg-declare-arg.ll

4 ↗

(On Diff #115480)

According to the radar, the testcase was:

class A { public: int x; int y; int z; int o; ~A() { x = 1; }};

A foo(int i) {
  int j = 0;
  if (i == 42) {
    j = i + 1;
  };
  A a;
  a.x = j;
  return a;
}

Can you verify that end-to-end, this still works?
Apparently older versions of the testcase never checked for the expressions so it is hard to tell what it originally was.

[aprantl]

Do you think you could craft a debuginfo-tests test from PR34513, so we can avoid regressing in the future?

[rnk]

In D37929#872707, @aprantl wrote:

Do you think you could craft a debuginfo-tests test from PR34513, so we can avoid regressing in the future?

Yep, once we have -O0 and -O1 working I'll commit one that tests both cases.

[aprantl]

Awesome.

[rnk]

llvm/test/DebugInfo/X86/dbg-declare-arg.ll

4 ↗

(On Diff #115480)

I verified this does the right thing now on gdb on Linux.

llvm/trunk/include/llvm/CodeGen/MachineInstrBuilder.h

MachineInstrBuilder BuildMI(MachineBasicBlock &BB,
                             unsigned Reg, const MDNode *Variable,
                             const MDNode *Expr);
 
 /// Clone a DBG_VALUE whose value has been spilled to FrameIndex.
 MachineInstr *buildDbgValueForSpill(MachineBasicBlock &BB,
                                     MachineBasicBlock::iterator I,
                                     const MachineInstr &Orig, int FrameIndex);
 
+/// Update a DBG_VALUE whose value has been spilled to FrameIndex. Useful when
+/// modifying an instruction in place while iterating over a basic block.
+void updateDbgValueForSpill(MachineInstr &Orig, int FrameIndex);
+
 inline unsigned getDefRegState(bool B) {
   return B ? RegState::Define : 0;
 }
 inline unsigned getImplRegState(bool B) {
   return B ? RegState::Implicit : 0;
 }
 inline unsigned getKillRegState(bool B) {
   return B ? RegState::Kill : 0;

llvm/trunk/lib/CodeGen/MachineInstr.cpp

MachineInstrBuilder llvm::BuildMI(MachineBasicBlock &BB,
   assert(isa<DILocalVariable>(Variable) && "not a variable");
   assert(cast<DIExpression>(Expr)->isValid() && "not an expression");
   MachineFunction &MF = *BB.getParent();
   MachineInstr *MI = BuildMI(MF, DL, MCID, IsIndirect, Reg, Variable, Expr);
   BB.insert(I, MI);
   return MachineInstrBuilder(MF, MI);
 }
 
+/// Compute the new DIExpression to use with a DBG_VALUE for a spill slot.
+/// This prepends DW_OP_deref when spilling an indirect DBG_VALUE.
+static const DIExpression *computeExprForSpill(const MachineInstr &MI) {
+  assert(MI.getOperand(0).isReg() && "can't spill non-register");
+  assert(MI.getDebugVariable()->isValidLocationForIntrinsic(MI.getDebugLoc()) &&
+         "Expected inlined-at fields to agree");
+
+  const DIExpression *Expr = MI.getDebugExpression();
+  if (MI.isIndirectDebugValue()) {
+    assert(MI.getOperand(1).getImm() == 0 && "DBG_VALUE with nonzero offset");
+    Expr = DIExpression::prepend(Expr, DIExpression::WithDeref);
+  }
+  return Expr;
+}
+
 MachineInstr *llvm::buildDbgValueForSpill(MachineBasicBlock &BB,
                                           MachineBasicBlock::iterator I,
                                           const MachineInstr &Orig,
                                           int FrameIndex) {
-  const MDNode *Var = Orig.getDebugVariable();
-  const auto *Expr = cast_or_null<DIExpression>(Orig.getDebugExpression());
-  bool IsIndirect = Orig.isIndirectDebugValue();
-  if (IsIndirect)
-    assert(Orig.getOperand(1).getImm() == 0 && "DBG_VALUE with nonzero offset");
-  DebugLoc DL = Orig.getDebugLoc();
-  assert(cast<DILocalVariable>(Var)->isValidLocationForIntrinsic(DL) &&
-         "Expected inlined-at fields to agree");
-  // If the DBG_VALUE already was a memory location, add an extra
-  // DW_OP_deref. Otherwise just turning this from a register into a
-  // memory/indirect location is sufficient.
-  if (IsIndirect)
-    Expr = DIExpression::prepend(Expr, DIExpression::WithDeref);
-  return BuildMI(BB, I, DL, Orig.getDesc())
+  const DIExpression *Expr = computeExprForSpill(Orig);
+  return BuildMI(BB, I, Orig.getDebugLoc(), Orig.getDesc())
       .addFrameIndex(FrameIndex)
       .addImm(0U)
-      .addMetadata(Var)
+      .addMetadata(Orig.getDebugVariable())
       .addMetadata(Expr);
 }
+
+void llvm::updateDbgValueForSpill(MachineInstr &Orig, int FrameIndex) {
+  const DIExpression *Expr = computeExprForSpill(Orig);
+  Orig.getOperand(0).ChangeToFrameIndex(FrameIndex);
+  Orig.getOperand(1).ChangeToImmediate(0U);
+  Orig.getOperand(3).setMetadata(Expr);
+}

llvm/trunk/lib/CodeGen/RegAllocFast.cpp

for (MachineInstr &MI : MBB) {
     const MCInstrDesc &MCID = MI.getDesc();
     DEBUG(
       dbgs() << "\n>> " << MI << "Regs:";
       dumpState()
     );
 
     // Debug values are not allowed to change codegen in any way.
     if (MI.isDebugValue()) {
-      bool ScanDbgValue = true;
       MachineInstr *DebugMI = &MI;
-      while (ScanDbgValue) {
-        ScanDbgValue = false;
-        for (unsigned I = 0, E = DebugMI->getNumOperands(); I != E; ++I) {
-          MachineOperand &MO = DebugMI->getOperand(I);
-          if (!MO.isReg()) continue;
-          unsigned Reg = MO.getReg();
-          if (!TargetRegisterInfo::isVirtualRegister(Reg)) continue;
-          LiveRegMap::iterator LRI = findLiveVirtReg(Reg);
-          if (LRI != LiveVirtRegs.end())
-            setPhysReg(*DebugMI, I, LRI->PhysReg);
-          else {
-            int SS = StackSlotForVirtReg[Reg];
-            if (SS == -1) {
-              // We can't allocate a physreg for a DebugValue, sorry!
-              DEBUG(dbgs() << "Unable to allocate vreg used by DBG_VALUE");
-              MO.setReg(0);
-            }
-            else {
-              // Modify DBG_VALUE now that the value is in a spill slot.
-              bool IsIndirect = DebugMI->isIndirectDebugValue();
-              if (IsIndirect)
-                assert(DebugMI->getOperand(1).getImm() == 0 &&
-                       "DBG_VALUE with nonzero offset");
-              const MDNode *Var = DebugMI->getDebugVariable();
-              const MDNode *Expr = DebugMI->getDebugExpression();
-              DebugLoc DL = DebugMI->getDebugLoc();
-              MachineBasicBlock *MBB = DebugMI->getParent();
-              assert(
-                  cast<DILocalVariable>(Var)->isValidLocationForIntrinsic(DL) &&
-                  "Expected inlined-at fields to agree");
-              MachineInstr *NewDV = BuildMI(*MBB, MBB->erase(DebugMI), DL,
-                                            TII->get(TargetOpcode::DBG_VALUE))
-                                        .addFrameIndex(SS)
-                                        .addImm(0U)
-                                        .addMetadata(Var)
-                                        .addMetadata(Expr);
-              DEBUG(dbgs() << "Modifying debug info due to spill:"
-                           << "\t" << *NewDV);
-              // Scan NewDV operands from the beginning.
-              DebugMI = NewDV;
-              ScanDbgValue = true;
-              break;
-            }
-          }
-          LiveDbgValueMap[Reg].push_back(DebugMI);
-        }
-      }
-      // Next instruction.
+      MachineOperand &MO = DebugMI->getOperand(0);
+
+      // Ignore DBG_VALUEs that aren't based on virtual registers. These are
+      // mostly constants and frame indices.
+      if (!MO.isReg())
+        continue;
+      if (!TargetRegisterInfo::isVirtualRegister(Reg))
+        continue;
+
+      // See if this virtual register has already been allocated to a physical
+      // register or spilled to a stack slot.
+        setPhysReg(*DebugMI, 0, LRI->PhysReg);
+        if (SS != -1) {
+          updateDbgValueForSpill(*DebugMI, SS);
+                       << "\t" << *DebugMI);
+          continue;
+
+        // We can't allocate a physreg for a DebugValue, sorry!
+        DEBUG(dbgs() << "Unable to allocate vreg used by DBG_VALUE");
+        MO.setReg(0);
+
+      // If Reg hasn't been spilled, put this DBG_VALUE in LiveDbgValueMap so
+      // that future spills of Reg will have DBG_VALUEs.
       continue;
     }
 
     // If this is a copy, we may be able to coalesce.
     unsigned CopySrcReg = 0;
     unsigned CopyDstReg = 0;
     unsigned CopySrcSub = 0;
     unsigned CopyDstSub = 0;

llvm/trunk/test/DebugInfo/X86/dbg-declare-arg.ll

 ; RUN: llc -O0 -fast-isel=true  -filetype=obj -o - %s | llvm-dwarfdump -v - | FileCheck %s
+
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
 target triple = "x86_64-apple-macosx10.6.7"
 ; rdar://problem/9321650
-;
+
+; C++ source:
+; class A { public: int x; int y; int z; int o; ~A() { x = 1; }};
+; 
+; A foo(int i) {
+;   int j = 0;
+;   if (i == 42) {
+;     j = i + 1;
+;   };
+;   A my_a;
+;   my_a.x = j;
+;   return my_a;
+; }
+
 ; CHECK: DW_AT_name {{.*}}"j"
 ; CHECK: DW_TAG_variable  
 ; CHECK-NEXT:   DW_AT_location [DW_FORM_sec_offset] (
-; CHECK-NEXT:     0x{{.*}} - 0x{{.*}}: DW_OP_breg7 RSP+8)
+; CHECK-NEXT:     0x{{.*}} - 0x{{.*}}: DW_OP_breg7 RSP+8, DW_OP_deref)
 ; CHECK-NEXT:   DW_AT_name {{.*}}"my_a"
 
 %class.A = type { i32, i32, i32, i32 }
 
 define void @_Z3fooi(%class.A* sret %agg.result, i32 %i) ssp !dbg !19 {
 entry:
   %i.addr = alloca i32, align 4
   %j = alloca i32, align 4