Re-land "[DebugInfo] Insert DW_OP_deref when spilling indirect DBG_VA…

…LUEs"

After r313775, it's easier to maintain a parallel BitVector of spilled
locations indexed by location number.

I wasn't able to build a good reduced test case for this iteration of
the bug, but I added a more direct assertion that spilled values must
use frame index locations. If this bug reappears, it won't only fire on
the NEON vector code that we detected it on, but on medium-sized
integer-only programs as well.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@313786 91177308-0d34-0410-b5e6-96231b3b80d8

lib/CodeGen/LiveDebugVariables.cpp

@@ -108,8 +108,8 @@ class LDVImpl;
 /// held by the same virtual register. The equivalence class is the transitive
 /// closure of that relation.
 class UserValue {
-  const MDNode *Variable;   ///< The debug info variable we are part of.
-  const MDNode *Expression; ///< Any complex address expression.
+  const DILocalVariable *Variable; ///< The debug info variable we are part of.
+  const DIExpression *Expression; ///< Any complex address expression.
   bool IsIndirect;        ///< true if this is a register-indirect+offset value.
   DebugLoc dl;            ///< The debug location for the variable. This is
                           ///< used by dwarf writer to find lexical scope.
@@ -127,8 +127,9 @@ class UserValue {
   SmallSet<SlotIndex, 2> trimmedDefs;
 
   /// insertDebugValue - Insert a DBG_VALUE into MBB at Idx for LocNo.
-  void insertDebugValue(MachineBasicBlock *MBB, SlotIndex Idx, unsigned LocNo,
-                        LiveIntervals &LIS, const TargetInstrInfo &TII);
+  void insertDebugValue(MachineBasicBlock *MBB, SlotIndex Idx,
+                        unsigned LocNo, bool Spilled, LiveIntervals &LIS,
+                        const TargetInstrInfo &TII);
 
   /// splitLocation - Replace OldLocNo ranges with NewRegs ranges where NewRegs
   /// is live. Returns true if any changes were made.
@@ -137,8 +138,8 @@ class UserValue {
 
 public:
   /// UserValue - Create a new UserValue.
-  UserValue(const MDNode *var, const MDNode *expr, bool i, DebugLoc L,
-            LocMap::Allocator &alloc)
+  UserValue(const DILocalVariable *var, const DIExpression *expr, bool i,
+            DebugLoc L, LocMap::Allocator &alloc)
       : Variable(var), Expression(expr), IsIndirect(i), dl(std::move(L)),
         leader(this), locInts(alloc) {}
 
@@ -154,8 +155,8 @@ class UserValue {
   UserValue *getNext() const { return next; }
 
   /// match - Does this UserValue match the parameters?
-  bool match(const MDNode *Var, const MDNode *Expr, const DILocation *IA,
-             bool indirect) const {
+  bool match(const DILocalVariable *Var, const DIExpression *Expr,
+             const DILocation *IA, bool indirect) const {
     return Var == Variable && Expr == Expression && dl->getInlinedAt() == IA &&
            indirect == IsIndirect;
   }
@@ -259,12 +260,14 @@ class UserValue {
                      LiveIntervals &LIS);
 
   /// rewriteLocations - Rewrite virtual register locations according to the
-  /// provided virtual register map.
-  void rewriteLocations(VirtRegMap &VRM, const TargetRegisterInfo &TRI);
+  /// provided virtual register map. Record which locations were spilled.
+  void rewriteLocations(VirtRegMap &VRM, const TargetRegisterInfo &TRI,
+                        BitVector &SpilledLocations);
 
   /// emitDebugValues - Recreate DBG_VALUE instruction from data structures.
-  void emitDebugValues(VirtRegMap *VRM,
-                       LiveIntervals &LIS, const TargetInstrInfo &TRI);
+  void emitDebugValues(VirtRegMap *VRM, LiveIntervals &LIS,
+                       const TargetInstrInfo &TRI,
+                       const BitVector &SpilledLocations);
 
   /// getDebugLoc - Return DebugLoc of this UserValue.
   DebugLoc getDebugLoc() { return dl;}
@@ -294,11 +297,11 @@ class LDVImpl {
   VRMap virtRegToEqClass;
 
   /// Map user variable to eq class leader.
-  using UVMap = DenseMap<const MDNode *, UserValue *>;
+  using UVMap = DenseMap<const DILocalVariable *, UserValue *>;
   UVMap userVarMap;
 
   /// getUserValue - Find or create a UserValue.
-  UserValue *getUserValue(const MDNode *Var, const MDNode *Expr,
+  UserValue *getUserValue(const DILocalVariable *Var, const DIExpression *Expr,
                           bool IsIndirect, const DebugLoc &DL);
 
   /// lookupVirtReg - Find the EC leader for VirtReg or null.
@@ -423,8 +426,9 @@ void UserValue::mapVirtRegs(LDVImpl *LDV) {
       LDV->mapVirtReg(locations[i].getReg(), this);
 }
 
-UserValue *LDVImpl::getUserValue(const MDNode *Var, const MDNode *Expr,
-                                 bool IsIndirect, const DebugLoc &DL) {
+UserValue *LDVImpl::getUserValue(const DILocalVariable *Var,
+                                 const DIExpression *Expr, bool IsIndirect,
+                                 const DebugLoc &DL) {
   UserValue *&Leader = userVarMap[Var];
   if (Leader) {
     UserValue *UV = Leader->getLeader();
@@ -463,11 +467,11 @@ bool LDVImpl::handleDebugValue(MachineInstr &MI, SlotIndex Idx) {
   }
 
   // Get or create the UserValue for (variable,offset).
-  bool IsIndirect = MI.isIndirectDebugValue();
+  bool IsIndirect = MI.getOperand(1).isImm();
   if (IsIndirect)
     assert(MI.getOperand(1).getImm() == 0 && "DBG_VALUE with nonzero offset");
-  const MDNode *Var = MI.getDebugVariable();
-  const MDNode *Expr = MI.getDebugExpression();
+  const DILocalVariable *Var = MI.getDebugVariable();
+  const DIExpression *Expr = MI.getDebugExpression();
   //here.
   UserValue *UV = getUserValue(Var, Expr, IsIndirect, MI.getDebugLoc());
   UV->addDef(Idx, MI.getOperand(0));
@@ -940,15 +944,20 @@ splitRegister(unsigned OldReg, ArrayRef<unsigned> NewRegs, LiveIntervals &LIS) {
     static_cast<LDVImpl*>(pImpl)->splitRegister(OldReg, NewRegs);
 }
 
-void UserValue::rewriteLocations(VirtRegMap &VRM,
-                                 const TargetRegisterInfo &TRI) {
+void UserValue::rewriteLocations(VirtRegMap &VRM, const TargetRegisterInfo &TRI,
+                                 BitVector &SpilledLocations) {
   // Build a set of new locations with new numbers so we can coalesce our
   // IntervalMap if two vreg intervals collapse to the same physical location.
   // Use MapVector instead of SetVector because MapVector::insert returns the
-  // position of the previously or newly inserted element.
+  // position of the previously or newly inserted element. The boolean value
+  // tracks if the location was produced by a spill.
+  // FIXME: This will be problematic if we ever support direct and indirect
+  // frame index locations, i.e. expressing both variables in memory and
+  // 'int x, *px = &x'. The "spilled" bit must become part of the location.
   MapVector<MachineOperand, bool> NewLocations;
   SmallVector<unsigned, 4> LocNoMap(locations.size());
   for (unsigned I = 0, E = locations.size(); I != E; ++I) {
+    bool Spilled = false;
     MachineOperand Loc = locations[I];
     // Only virtual registers are rewritten.
     if (Loc.isReg() && Loc.getReg() &&
@@ -963,6 +972,7 @@ void UserValue::rewriteLocations(VirtRegMap &VRM,
       } else if (VRM.getStackSlot(VirtReg) != VirtRegMap::NO_STACK_SLOT) {
         // FIXME: Translate SubIdx to a stackslot offset.
         Loc = MachineOperand::CreateFI(VRM.getStackSlot(VirtReg));
+        Spilled = true;
       } else {
         Loc.setReg(0);
         Loc.setSubReg(0);
@@ -971,14 +981,22 @@ void UserValue::rewriteLocations(VirtRegMap &VRM,
 
     // Insert this location if it doesn't already exist and record a mapping
     // from the old number to the new number.
-    auto InsertResult = NewLocations.insert({Loc, false});
-    LocNoMap[I] = std::distance(NewLocations.begin(), InsertResult.first);
+    auto InsertResult = NewLocations.insert({Loc, Spilled});
+    unsigned NewLocNo = std::distance(NewLocations.begin(), InsertResult.first);
+    LocNoMap[I] = NewLocNo;
   }
 
-  // Rewrite the locations.
+  // Rewrite the locations and record which ones were spill slots.
   locations.clear();
-  for (const auto &Pair : NewLocations)
+  SpilledLocations.clear();
+  SpilledLocations.resize(NewLocations.size());
+  for (auto &Pair : NewLocations) {
     locations.push_back(Pair.first);
+    if (Pair.second) {
+      unsigned NewLocNo = std::distance(&*NewLocations.begin(), &Pair);
+      SpilledLocations.set(NewLocNo);
+    }
+  }
 
   // Update the interval map, but only coalesce left, since intervals to the
   // right use the old location numbers. This should merge two contiguous
@@ -1016,7 +1034,7 @@ findInsertLocation(MachineBasicBlock *MBB, SlotIndex Idx,
 }
 
 void UserValue::insertDebugValue(MachineBasicBlock *MBB, SlotIndex Idx,
-                                 unsigned LocNo,
+                                 unsigned LocNo, bool Spilled,
                                  LiveIntervals &LIS,
                                  const TargetInstrInfo &TII) {
   MachineBasicBlock::iterator I = findInsertLocation(MBB, Idx, LIS);
@@ -1026,25 +1044,38 @@ void UserValue::insertDebugValue(MachineBasicBlock *MBB, SlotIndex Idx,
   assert(cast<DILocalVariable>(Variable)
              ->isValidLocationForIntrinsic(getDebugLoc()) &&
          "Expected inlined-at fields to agree");
-  if (Loc.isReg())
-    BuildMI(*MBB, I, getDebugLoc(), TII.get(TargetOpcode::DBG_VALUE),
-            IsIndirect, Loc.getReg(), Variable, Expression);
+
+  // If the location was spilled, the new DBG_VALUE will be indirect. If the
+  // original DBG_VALUE was indirect, we need to add DW_OP_deref to indicate
+  // that the original virtual register was a pointer.
+  bool NewIndirect = IsIndirect || Spilled;
+  const DIExpression *Expr = Expression;
+  if (Spilled && IsIndirect)
+    Expr = DIExpression::prepend(Expr, DIExpression::WithDeref);
+
+  assert((!Spilled || Loc.isFI()) &&
+         "a spilled location must be a frame index");
+
+  MachineInstrBuilder MIB =
+      BuildMI(*MBB, I, getDebugLoc(), TII.get(TargetOpcode::DBG_VALUE))
+          .add(Loc);
+  if (NewIndirect)
+    MIB.addImm(0U);
   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);
 }
 
 void UserValue::emitDebugValues(VirtRegMap *VRM, LiveIntervals &LIS,
-                                const TargetInstrInfo &TII) {
+                                const TargetInstrInfo &TII,
+                                const BitVector &SpilledLocations) {
   MachineFunction::iterator MFEnd = VRM->getMachineFunction().end();
 
   for (LocMap::const_iterator I = locInts.begin(); I.valid();) {
     SlotIndex Start = I.start();
     SlotIndex Stop = I.stop();
     unsigned LocNo = I.value();
+    bool Spilled = LocNo != UndefLocNo ? SpilledLocations.test(LocNo) : false;
 
     // If the interval start was trimmed to the lexical scope insert the
     // DBG_VALUE at the previous index (otherwise it appears after the
@@ -1057,7 +1088,7 @@ void UserValue::emitDebugValues(VirtRegMap *VRM, LiveIntervals &LIS,
     SlotIndex MBBEnd = LIS.getMBBEndIdx(&*MBB);
 
     DEBUG(dbgs() << " BB#" << MBB->getNumber() << '-' << MBBEnd);
-    insertDebugValue(&*MBB, Start, LocNo, LIS, TII);
+    insertDebugValue(&*MBB, Start, LocNo, Spilled, LIS, TII);
     // This interval may span multiple basic blocks.
     // Insert a DBG_VALUE into each one.
     while(Stop > MBBEnd) {
@@ -1067,7 +1098,7 @@ void UserValue::emitDebugValues(VirtRegMap *VRM, LiveIntervals &LIS,
         break;
       MBBEnd = LIS.getMBBEndIdx(&*MBB);
       DEBUG(dbgs() << " BB#" << MBB->getNumber() << '-' << MBBEnd);
-      insertDebugValue(&*MBB, Start, LocNo, LIS, TII);
+      insertDebugValue(&*MBB, Start, LocNo, Spilled, LIS, TII);
     }
     DEBUG(dbgs() << '\n');
     if (MBB == MFEnd)
@@ -1082,10 +1113,11 @@ void LDVImpl::emitDebugValues(VirtRegMap *VRM) {
   if (!MF)
     return;
   const TargetInstrInfo *TII = MF->getSubtarget().getInstrInfo();
+  BitVector SpilledLocations;
   for (unsigned i = 0, e = userValues.size(); i != e; ++i) {
     DEBUG(userValues[i]->print(dbgs(), TRI));
-    userValues[i]->rewriteLocations(*VRM, *TRI);
-    userValues[i]->emitDebugValues(VRM, *LIS, *TII);
+    userValues[i]->rewriteLocations(*VRM, *TRI, SpilledLocations);
+    userValues[i]->emitDebugValues(VRM, *LIS, *TII, SpilledLocations);
   }
   EmitDone = true;
 }

test/DebugInfo/X86/bbjoin.ll

@@ -11,12 +11,12 @@
 ; }
 ; CHECK: ![[X:.*]] = !DILocalVariable(name: "x",
 ; CHECK: bb.0.entry:
-; CHECK:   DBG_VALUE 23, 0, ![[X]],
+; CHECK:   DBG_VALUE 23, debug-use _, ![[X]],
 ; CHECK:   DBG_VALUE %rsp, 0, ![[X]], !DIExpression(DW_OP_plus_uconst, 4, DW_OP_deref),
 ; CHECK: bb.1.if.then:
-; CHECK:   DBG_VALUE 43, 0, ![[X]],
+; CHECK:   DBG_VALUE 43, debug-use _, ![[X]],
 ; CHECK: bb.2.if.end:
-; CHECK-NOT:  DBG_VALUE 23, 0, ![[X]],
+; CHECK-NOT:  DBG_VALUE 23, debug-use _, ![[X]],
 ; CHECK:   RETQ %eax
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"

test/DebugInfo/X86/spill-indirect-nrvo.ll

@@ -0,0 +1,103 @@
+; RUN: llc < %s | FileCheck %s
+; RUN: llc -O0 < %s | FileCheck %s
+
+; Make sure we insert DW_OP_deref when spilling indirect DBG_VALUE instructions.
+
+; C++ source:
+; #define FORCE_SPILL() \
+;   __asm volatile("" : : : \
+;                    "rax", "rbx", "rcx", "rdx", "rsi", "rdi", "rbp", "r8", \
+;                    "r9", "r10", "r11", "r12", "r13", "r14", "r15")
+; struct string {
+;   string();
+;   string(int i);
+;   ~string();
+;   int i = 0;
+; };
+; string get_string() {
+;   string result = 3;
+;   FORCE_SPILL();
+;   return result;
+; }
+
+; CHECK-LABEL: _Z10get_stringv:
+; CHECK: #DEBUG_VALUE: get_string:result <- [%RDI+0]
+; CHECK: movq   %rdi, [[OFFS:[0-9]+]](%rsp)          # 8-byte Spill
+; CHECK: #DEBUG_VALUE: get_string:result <- [DW_OP_plus_uconst [[OFFS]], DW_OP_deref] [%RSP+0]
+; CHECK: callq  _ZN6stringC1Ei
+; CHECK: #APP
+; CHECK: #NO_APP
+
+; ModuleID = 't.cpp'
+source_filename = "t.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64--linux"
+
+%struct.string = type { i32 }
+
+; Function Attrs: uwtable
+define void @_Z10get_stringv(%struct.string* noalias sret %agg.result) #0 !dbg !7 {
+entry:
+  %nrvo = alloca i1, align 1
+  store i1 false, i1* %nrvo, align 1, !dbg !24
+  call void @llvm.dbg.declare(metadata %struct.string* %agg.result, metadata !23, metadata !DIExpression()), !dbg !25
+  call void @_ZN6stringC1Ei(%struct.string* %agg.result, i32 3), !dbg !26
+  call void asm sideeffect "", "~{rax},~{rbx},~{rcx},~{rdx},~{rsi},~{rdi},~{rbp},~{r8},~{r9},~{r10},~{r11},~{r12},~{r13},~{r14},~{r15},~{dirflag},~{fpsr},~{flags}"() #3, !dbg !27, !srcloc !28
+  store i1 true, i1* %nrvo, align 1, !dbg !29
+  %nrvo.val = load i1, i1* %nrvo, align 1, !dbg !30
+  br i1 %nrvo.val, label %nrvo.skipdtor, label %nrvo.unused, !dbg !30
+
+nrvo.unused:                                      ; preds = %entry
+  call void @_ZN6stringD1Ev(%struct.string* %agg.result), !dbg !30
+  br label %nrvo.skipdtor, !dbg !30
+
+nrvo.skipdtor:                                    ; preds = %nrvo.unused, %entry
+  ret void, !dbg !30
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+declare void @_ZN6stringC1Ei(%struct.string*, i32) unnamed_addr
+
+declare void @_ZN6stringD1Ev(%struct.string*) unnamed_addr
+
+attributes #0 = { uwtable }
+attributes #1 = { nounwind readnone speculatable }
+attributes #3 = { nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 6.0.0 ", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "t.cpp", directory: "C:\5Csrc\5Cllvm-project\5Cbuild")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 6.0.0 "}
+!7 = distinct !DISubprogram(name: "get_string", linkageName: "_Z10get_stringv", scope: !1, file: !1, line: 13, type: !8, isLocal: false, isDefinition: true, scopeLine: 13, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !22)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "string", file: !1, line: 7, size: 32, elements: !11, identifier: "_ZTS6string")
+!11 = !{!12, !14, !18, !21}
+!12 = !DIDerivedType(tag: DW_TAG_member, name: "i", scope: !10, file: !1, line: 11, baseType: !13, size: 32)
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!14 = !DISubprogram(name: "string", scope: !10, file: !1, line: 8, type: !15, isLocal: false, isDefinition: false, scopeLine: 8, flags: DIFlagPrototyped, isOptimized: true)
+!15 = !DISubroutineType(types: !16)
+!16 = !{null, !17}
+!17 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !10, size: 64, flags: DIFlagArtificial | DIFlagObjectPointer)
+!18 = !DISubprogram(name: "string", scope: !10, file: !1, line: 9, type: !19, isLocal: false, isDefinition: false, scopeLine: 9, flags: DIFlagPrototyped, isOptimized: true)
+!19 = !DISubroutineType(types: !20)
+!20 = !{null, !17, !13}
+!21 = !DISubprogram(name: "~string", scope: !10, file: !1, line: 10, type: !15, isLocal: false, isDefinition: false, scopeLine: 10, flags: DIFlagPrototyped, isOptimized: true)
+!22 = !{!23}
+!23 = !DILocalVariable(name: "result", scope: !7, file: !1, line: 14, type: !10)
+!24 = !DILocation(line: 14, column: 3, scope: !7)
+!25 = !DILocation(line: 14, column: 10, scope: !7)
+!26 = !DILocation(line: 14, column: 19, scope: !7)
+!27 = !DILocation(line: 15, column: 3, scope: !7)
+!28 = !{i32 -2147471175}
+!29 = !DILocation(line: 16, column: 3, scope: !7)
+!30 = !DILocation(line: 17, column: 1, scope: !7)