[DebugInfo] Handle memory locations in LiveDebugValues

[rnk]

Summary

Track the DIExpression that contains the offset and use it when
comparing variable locations.

[aprantl]

First off: Thanks!
Does this have have any performance impact? If have the time, it would be nice to just compare the wall clock time of building an asanified (that should generate lots of memory locations) llc or clang with and without the patch.

llvm/lib/CodeGen/LiveDebugValues.cpp

124

I wouldn't call it offset if it's the entire expression

[aprantl]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5173

I think this should be a separate commit?

[rnk]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5173

Ugh, this doesn't actually do the right thing. Here's my test that comes here:

void useptr(int *);
void use_alloca(int cond) {
  int x = 0;
  int *px = &x;
  if (cond)
    useptr(px);
}

After optimization, px will be described by a dbg.value of x's alloca. This code in InstrEmitter.cpp incorrectly builds an "indirect in memory" DBG_VALUE for px:

if (SD->getKind() == SDDbgValue::FRAMEIX) {
  // Stack address; this needs to be lowered in target-dependent fashion.
  // EmitTargetCodeForFrameDebugValue is responsible for allocation.
  return BuildMI(*MF, DL, TII->get(TargetOpcode::DBG_VALUE))
      .addFrameIndex(SD->getFrameIx())
      .addImm(0)
      .addMetadata(Var)
      .addMetadata(Expr);
}

I guess I'll need to fix that to look for DW_OP_deref.

[rnk]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5173

Alternatively, you could say I misunderstood DAG.getFrameIndexDbgValue.

[zturner]

Can you explain this in terms of an actual debugger scenario that this improves? If we were debugging the code in your test case:

int join_memlocs(int cond) {
  int x = getint();
  if (cond) {
    x = 42;     // Dead store, can delete
    dostuff(x); // Can propagate 42
    x = getint();
  }
  escape(&x); // Escape variable to force it in memory
  return x;
}

What would be the debugger's behavior before the patch and after the patch? And did you check (e.g. with MSVC) that there's actually a discernible result in the debugger experience?

[aprantl]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5173

I think it would be best to split the SelectionDAG change out into a separate review and have a MIR-only test for LiveDebugValues. The LiveDebugValues change should be generally good and uncontroversial (apart from performance considerations) and this change looks like it will generate a longer discussion.

[rnk]

In D37078#851603, @zturner wrote:

Can you explain this in terms of an actual debugger scenario that this improves?

No, but this is a necessary pre-requisite to http://llvm.org/pr34136, starting from the bottom up. Clang doesn't generate the dbg.values used in these test cases yet because LLVM doesn't handle them, but it will soon.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5173

Agreed.

llvm/lib/CodeGen/LiveDebugValues.cpp

 
 namespace {
 
 // \brief If @MI is a DBG_VALUE with debug value described by a defined
 // register, returns the number of this register. In the other case, returns 0.
 static unsigned isDbgValueDescribedByReg(const MachineInstr &MI) {
   assert(MI.isDebugValue() && "expected a DBG_VALUE");
   assert(MI.getNumOperands() == 4 && "malformed DBG_VALUE");
-  // If location of variable is described using a register (directly
-  // or indirectly), this register is always a first operand.
+
+  // If the second operand is an immediate value, this must live in memory. The
+  // offset in the DBG_VALUE instruction is captured by the DIExpression.
+  if (MI.getOperand(1).isImm())
+    return 0;
+
+  // If location of variable is described directly using a register, this
+  // register is always a first operand.
   return MI.getOperand(0).isReg() ? MI.getOperand(0).getReg() : 0;
 }
 
 class LiveDebugValues : public MachineFunctionPass {
 
 private:
   const TargetRegisterInfo *TRI;
   const TargetInstrInfo *TII;
struct DebugVariable : public DebugVariableBase {
     }
   };
 
   /// A pair of debug variable and value location.
   struct VarLoc {
     const DebugVariable Var;
     const MachineInstr &MI; ///< Only used for cloning a new DBG_VALUE.
     mutable UserValueScopes UVS;
-    enum { InvalidKind = 0, RegisterKind } Kind;
+    enum { InvalidKind = 0, RegisterKind, MemoryKind } Kind;
 
     /// The value location. Stored separately to avoid repeatedly
     /// extracting it from MI.
-    union {
-      uint64_t RegNo;
-      uint64_t Hash;
-    } Loc;
+    uint64_t RegNo = 0;
+    const DIExpression *ExprOffset = nullptr;
 
     VarLoc(const MachineInstr &MI, LexicalScopes &LS)
         : Var(MI.getDebugVariable(), MI.getDebugLoc()->getInlinedAt()), MI(MI),
           UVS(MI.getDebugLoc(), LS), Kind(InvalidKind) {
-      static_assert((sizeof(Loc) == sizeof(uint64_t)),
-                    "hash does not cover all members of Loc");
       assert(MI.isDebugValue() && "not a DBG_VALUE");
       assert(MI.getNumOperands() == 4 && "malformed DBG_VALUE");
-      if (int RegNo = isDbgValueDescribedByReg(MI)) {
+      if (MI.isIndirectDebugValue()) {
+        Kind = MemoryKind;
+        RegNo = MI.getOperand(0).getReg();
+        // FIXME: We should really extract the offset and treat it as an
+        // integer.
+        ExprOffset = cast<DIExpression>(MI.getDebugExpression());
+      } else if (int Reg = isDbgValueDescribedByReg(MI)) {
         Kind = RegisterKind;
-        Loc.RegNo = RegNo;
+        RegNo = Reg;
       }
     }
 
     /// If this variable is described by a register, return it,
     /// otherwise return 0.
     unsigned isDescribedByReg() const {
       if (Kind == RegisterKind)
-        return Loc.RegNo;
+        return RegNo;
       return 0;
     }
 
     /// Determine whether the lexical scope of this value's debug location
     /// dominates MBB.
     bool dominates(MachineBasicBlock &MBB) const { return UVS.dominates(&MBB); }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
     LLVM_DUMP_METHOD void dump() const { MI.dump(); }
 #endif
 
     bool operator==(const VarLoc &Other) const {
-      return Var == Other.Var && Loc.Hash == Other.Loc.Hash;
+      return Var == Other.Var && RegNo == Other.RegNo &&
+             ExprOffset == Other.ExprOffset;
     }
 
     /// This operator guarantees that VarLocs are sorted by Variable first.
     bool operator<(const VarLoc &Other) const {
-      if (Var == Other.Var)
-        return Loc.Hash < Other.Loc.Hash;
+      if (Var != Other.Var)
         return Var < Other.Var;
+      if (RegNo != Other.RegNo)
+        return RegNo < Other.RegNo;
+      return ExprOffset < Other.ExprOffset;
     }
   };
 
   typedef UniqueVector<VarLoc> VarLocMap;
   typedef SparseBitVector<> VarLocSet;
   typedef SmallDenseMap<const MachineBasicBlock *, VarLocSet> VarLocInMBB;
   struct SpillDebugPair {
     MachineInstr *SpillInst;
void LiveDebugValues::transferDebugValue(const MachineInstr &MI,
   assert(Var->isValidLocationForIntrinsic(DebugLoc) &&
          "Expected inlined-at fields to agree");
 
   // End all previous ranges of Var.
   DebugVariable V(Var, InlinedAt);
   OpenRanges.erase(V);
 
   // Add the VarLoc to OpenRanges from this DBG_VALUE.
-  // TODO: Currently handles DBG_VALUE which has only reg as location.
-  if (isDbgValueDescribedByReg(MI)) {
-    VarLoc VL(MI, LS);
+  if (VL.Kind != VarLoc::InvalidKind) {
     unsigned ID = VarLocIDs.insert(VL);
     OpenRanges.insert(ID, VL.Var);
   }
 }
 
 /// A definition of a register may mark the end of a range.
 void LiveDebugValues::transferRegisterDef(MachineInstr &MI,
                                           OpenRangesSet &OpenRanges,

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

case Intrinsic::dbg_value: {
 
     SDDbgValue *SDV;
     if (isa<ConstantInt>(V) || isa<ConstantFP>(V) || isa<UndefValue>(V)) {
       SDV = DAG.getConstantDbgValue(Variable, Expression, V, dl, SDNodeOrder);
       DAG.AddDbgValue(SDV, nullptr, false);
       return nullptr;
     }
 
+    // If the value is an alloca, get its frame index instead of looking in the
+    // node map.
+    // FIXME: Handle byval/inalloca params as above.
+    if (const AllocaInst *AI = dyn_cast<AllocaInst>(V)) {
+      auto Iter = FuncInfo.StaticAllocaMap.find(AI);
+      if (Iter != FuncInfo.StaticAllocaMap.end()) {
+        int FI = Iter->second;
+        SDV = DAG.getFrameIndexDbgValue(Variable, Expression, FI, dl,
+                                        SDNodeOrder);
+        DAG.AddDbgValue(SDV, nullptr, false);
+        return nullptr;
+      }
+    }
+
     // Do not use getValue() in here; we don't want to generate code at
     // this point if it hasn't been done yet.
     SDValue N = NodeMap[V];
     if (!N.getNode() && isa<Argument>(V)) // Check unused arguments map.
       N = UnusedArgNodeMap[V];
     if (N.getNode()) {
       if (EmitFuncArgumentDbgValue(V, Variable, Expression, dl, false, N))
         return nullptr;

llvm/test/DebugInfo/X86/live-debug-values-deref.ll

+; RUN: llc -stop-after=livedebugvalues %s -o - | FileCheck %s
+
+; Live debug values should be able to join two memory locations together in its
+; dataflow.
+
+; Original C source before modification to use dbg.value+DW_OP_deref, which we
+; wish to generate in the frontend eventually:
+; int getint(void);
+; void escape(int *);
+; int join_memlocs(int cond) {
+;   int x = getint();
+;   if (cond) {
+;     x = 42;     // Dead store, can delete
+;     dostuff(x); // Can propagate 42
+;     x = getint();
+;   }
+;   escape(&x); // Escape variable to force it in memory
+;   return x;
+; }
+
+; ModuleID = 't.c'
+source_filename = "t.c"
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc19.0.24215"
+
+define i32 @join_memlocs(i32 %cond) !dbg !8 {
+entry:
+  %x = alloca i32, align 4
+  tail call void @llvm.dbg.value(metadata i32* %x, metadata !14, metadata !DIExpression(DW_OP_deref)), !dbg !19
+  %call = tail call i32 @getint(), !dbg !18
+  store i32 %call, i32* %x, align 4, !dbg !19, !tbaa !20
+  %tobool = icmp eq i32 %cond, 0, !dbg !24
+  br i1 %tobool, label %if.end, label %if.then, !dbg !26
+
+if.then:                                          ; preds = %entry
+  tail call void @llvm.dbg.value(metadata i32 42, metadata !14, metadata !DIExpression()), !dbg !36
+  tail call void @useint(i32 42), !dbg !27
+  %call1 = tail call i32 @getint(), !dbg !29
+  store i32 %call1, i32* %x, align 4, !dbg !30, !tbaa !20
+  tail call void @llvm.dbg.value(metadata i32* %x, metadata !14, metadata !DIExpression(DW_OP_deref)), !dbg !30
+  br label %if.end, !dbg !31
+
+if.end:                                           ; preds = %entry, %if.then
+  call void @escape(i32* nonnull %x), !dbg !32
+  %rv = load i32, i32* %x, align 4, !dbg !33, !tbaa !20
+  ret i32 %rv, !dbg !35
+}
+
+; CHECK-LABEL: name: join_memlocs
+; CHECK: body: |
+; CHECK:   bb.0.entry:
+; CHECK:     DBG_VALUE %rsp, 0, !{{[0-9]+}}, !DIExpression(DW_OP_plus_uconst, {{[0-9]+}}, DW_OP_deref), debug-location !{{[0-9]+}}
+; CHECK:   bb.1.if.then:
+; CHECK:     DBG_VALUE 42, 0, !{{[0-9]+}}, !DIExpression(), debug-location !{{[0-9]+}}
+; CHECK:   bb.2.if.end:
+; CHECK:     DBG_VALUE debug-use %rsp, 0, !{{[0-9]+}}, !DIExpression(DW_OP_plus_uconst, {{[0-9]+}}, DW_OP_deref), debug-location !{{[0-9]+}}
+
+
+; Make sure we account for the offset so we don't join unrelated memory locations.
+define i32 @join_fail(i32 %cond) !dbg !40 {
+entry:
+  %x = alloca i32, align 4
+  %y = alloca i32, align 4
+  tail call void @llvm.dbg.value(metadata i32* %x, metadata !42, metadata !DIExpression(DW_OP_deref)), !dbg !41
+  %call = tail call i32 @getint()
+  store i32 %call, i32* %x, align 4
+  %tobool = icmp eq i32 %cond, 0
+  br i1 %tobool, label %if.end, label %if.then
+
+if.then:                                          ; preds = %entry
+  tail call void @llvm.dbg.value(metadata i32 42, metadata !42, metadata !DIExpression()), !dbg !41
+  tail call void @useint(i32 42), !dbg !41
+  %call1 = tail call i32 @getint(), !dbg !41
+  store i32 %call1, i32* %x, align 4
+  tail call void @llvm.dbg.value(metadata i32* %y, metadata !42, metadata !DIExpression(DW_OP_deref)), !dbg !41
+  br label %if.end
+
+if.end:                                           ; preds = %entry, %if.then
+  call void @escape(i32* nonnull %x), !dbg !41
+  %rv = load i32, i32* %x, align 4
+  ret i32 %rv
+}
+
+; CHECK-LABEL: name: join_fail
+; CHECK: body: |
+; CHECK:   bb.0.entry:
+; CHECK:     DBG_VALUE %rsp, 0, !{{[0-9]+}}, !DIExpression(DW_OP_plus_uconst, {{[0-9]+}}, DW_OP_deref), debug-location !{{[0-9]+}}
+; CHECK:   bb.1.if.then:
+; CHECK:     DBG_VALUE 42, 0, !{{[0-9]+}}, !DIExpression(), debug-location !{{[0-9]+}}
+; CHECK:   bb.2.if.end:
+; CHECK-NOT:     DBG_VALUE
+
+
+declare i32 @getint()
+
+declare void @useint(i32)
+
+declare void @escape(i32*)
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 6.0.0 ", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "t.c", 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 2}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!7 = !{!"clang version 6.0.0 "}
+!8 = distinct !DISubprogram(name: "join_memlocs", scope: !1, file: !1, line: 4, type: !9, isLocal: false, isDefinition: true, scopeLine: 4, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !12)
+!9 = !DISubroutineType(types: !10)
+!10 = !{!11, !11}
+!11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!12 = !{!13, !14}
+!13 = !DILocalVariable(name: "cond", arg: 1, scope: !8, file: !1, line: 4, type: !11)
+!14 = !DILocalVariable(name: "x", scope: !8, file: !1, line: 5, type: !11)
+!15 = !DIExpression()
+!16 = !DILocation(line: 4, column: 11, scope: !8)
+!17 = !DILocation(line: 5, column: 3, scope: !8)
+!18 = !DILocation(line: 5, column: 11, scope: !8)
+!19 = !DILocation(line: 5, column: 7, scope: !8)
+!20 = !{!21, !21, i64 0}
+!21 = !{!"int", !22, i64 0}
+!22 = !{!"omnipotent char", !23, i64 0}
+!23 = !{!"Simple C/C++ TBAA"}
+!24 = !DILocation(line: 6, column: 7, scope: !25)
+!25 = distinct !DILexicalBlock(scope: !8, file: !1, line: 6, column: 7)
+!26 = !DILocation(line: 6, column: 7, scope: !8)
+!27 = !DILocation(line: 8, column: 5, scope: !28)
+!28 = distinct !DILexicalBlock(scope: !25, file: !1, line: 6, column: 13)
+!29 = !DILocation(line: 9, column: 9, scope: !28)
+!30 = !DILocation(line: 9, column: 7, scope: !28)
+!31 = !DILocation(line: 10, column: 3, scope: !28)
+!32 = !DILocation(line: 11, column: 3, scope: !8)
+!33 = !DILocation(line: 12, column: 10, scope: !8)
+!34 = !DILocation(line: 13, column: 1, scope: !8)
+!35 = !DILocation(line: 12, column: 3, scope: !8)
+!36 = !DILocation(line: 7, column: 5, scope: !28)
+
+!40 = distinct !DISubprogram(name: "join_memlocs", scope: !1, file: !1, line: 4, type: !9, isLocal: false, isDefinition: true, scopeLine: 4, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !12)
+!41 = !DILocation(line: 42, column: 0, scope: !40)
+!42 = !DILocalVariable(name: "x", scope: !40, file: !1, line: 42, type: !11)