[DebugInfo] LiveDebugValues: locations with different exprs should compare differently

[jmorse]

Summary

In the attached C test case, when joining variable locations from two parent blocks, the "baz" variable in one parent is located in a register with an empty DIExpression, in the other block it's located in the same register but with a complex expression. LiveDebugValues wrongly decides that this is the same location, and propagates one of the locations for "baz" to the exit block. The correct behaviour would be to propagate no location: the value of "baz" depends on the path taken through the program, and isn't stored in a register or stack slot.

Happily this is a simple as adding a DIExpression field to the "VarLoc" class (which identifies a machine location), and having VarLocs compare unequal if their DIExpressions are different. This relies on the DIExpressions being uniq'd, which I believe metadata guarantees.

On a clang-3.4 build this results in no variable losing all their locations, and a fall in 0.6% scope-bytes-covered. Which is great, because those are probably invalid locations.

[aprantl]

This makes sense, I'm kind of shocked that me missed something that obvious. Is there a big drop in variable locations with this (i.e., an improvement in accuracy)?

lib/CodeGen/LiveDebugValues.cpp

196 ↗

(On Diff #217864)

Do you need to store this here, or could you derive it from MI below?

[jmorse]

In D66942#1651751, @aprantl wrote:

Is there a big drop in variable locations with this (i.e., an improvement in accuracy)?

Zero change in the total number of variables with locations; but the number of bytes they cover falls by 0.6% (from 46.8% of total to 46.2%, IIRC). Not a huge change, but that's a decent number of locations that were presumably wrong.

lib/CodeGen/LiveDebugValues.cpp

196 ↗

(On Diff #217864)

It could be either; the comment on line 211 indicates past concern about repeatedly accessing MI when comparing VarLocs. I'll give both options a spin through an asan build of clang-3.4 to see if there's any real difference.

[jmorse]

lib/CodeGen/LiveDebugValues.cpp

196 ↗

(On Diff #217864)

There are only fractional performance differences between {unpatched, this patch, expr-derived-from-MI} clang-trunk when building clang-3.4 with asan. Typically <0.5%, which is more or less the margin of error on my machines. So, it could be either.

[aprantl]

lib/CodeGen/LiveDebugValues.cpp

195 ↗

(On Diff #217864)

remove (potentially complex)?

llvm/trunk/lib/CodeGen/LiveDebugValues.cpp

struct VarLoc {
     struct SpillLoc {
       unsigned SpillBase;
       int SpillOffset;
       bool operator==(const SpillLoc &Other) const {
         return SpillBase == Other.SpillBase && SpillOffset == Other.SpillOffset;
       }
     };
 
+    /// Identity of the variable at this location.
     const DebugVariable Var;
-    const MachineInstr &MI; ///< Only used for cloning a new DBG_VALUE.
+
+    /// The expression applied to this location.
+    const DIExpression *Expr;
+
+    /// DBG_VALUE to clone var/expr information from if this location
+    /// is moved.
+    const MachineInstr &MI;
+
     mutable UserValueScopes UVS;
     enum VarLocKind {
       InvalidKind = 0,
       RegisterKind,
       SpillLocKind,
       ImmediateKind,
       EntryValueKind
     } Kind = InvalidKind;
 
     /// The value location. Stored separately to avoid repeatedly
     /// extracting it from MI.
     union {
       uint64_t RegNo;
       SpillLoc SpillLocation;
       uint64_t Hash;
       int64_t Immediate;
       const ConstantFP *FPImm;
       const ConstantInt *CImm;
     } Loc;
 
     VarLoc(const MachineInstr &MI, LexicalScopes &LS,
            VarLocKind K = InvalidKind)
-        : Var(MI), MI(MI), UVS(MI.getDebugLoc(), LS){
+        : Var(MI), Expr(MI.getDebugExpression()), MI(MI),
+          UVS(MI.getDebugLoc(), LS) {
       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)) {
         Kind = MI.isDebugEntryValue() ? EntryValueKind : RegisterKind;
         Loc.RegNo = RegNo;
       } else if (MI.getOperand(0).isImm()) {
         Kind = ImmediateKind;
         Loc.Immediate = MI.getOperand(0).getImm();
       } else if (MI.getOperand(0).isFPImm()) {
         Kind = ImmediateKind;
         Loc.FPImm = MI.getOperand(0).getFPImm();
       } else if (MI.getOperand(0).isCImm()) {
         Kind = ImmediateKind;
         Loc.CImm = MI.getOperand(0).getCImm();
       }
       assert((Kind != ImmediateKind || !MI.isDebugEntryValue()) &&
              "entry values must be register locations");
     }
 
     /// The constructor for spill locations.
     VarLoc(const MachineInstr &MI, unsigned SpillBase, int SpillOffset,
            LexicalScopes &LS, const MachineInstr &OrigMI)
-        : Var(MI), MI(OrigMI), UVS(MI.getDebugLoc(), LS) {
+        : Var(MI), Expr(MI.getDebugExpression()), MI(OrigMI),
+          UVS(MI.getDebugLoc(), LS) {
       assert(MI.isDebugValue() && "not a DBG_VALUE");
       assert(MI.getNumOperands() == 4 && "malformed DBG_VALUE");
       Kind = SpillLocKind;
       Loc.SpillLocation = {SpillBase, SpillOffset};
     }
 
     // Is the Loc field a constant or constant object?
     bool isConstant() const { return Kind == ImmediateKind; }
struct VarLoc {
     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 Kind == Other.Kind && Var == Other.Var &&
-             Loc.Hash == Other.Loc.Hash;
+             Loc.Hash == Other.Loc.Hash && Expr == Other.Expr;
     }
 
     /// This operator guarantees that VarLocs are sorted by Variable first.
     bool operator<(const VarLoc &Other) const {
-      return std::tie(Var, Kind, Loc.Hash) <
-             std::tie(Other.Var, Other.Kind, Other.Loc.Hash);
+      return std::tie(Var, Kind, Loc.Hash, Expr) <
+             std::tie(Other.Var, Other.Kind, Other.Loc.Hash, Other.Expr);
     }
   };
 
   using DebugParamMap = SmallDenseMap<const DILocalVariable *, MachineInstr *>;
   using VarLocMap = UniqueVector<VarLoc>;
   using VarLocSet = SparseBitVector<>;
   using VarLocInMBB = SmallDenseMap<const MachineBasicBlock *, VarLocSet>;
   struct TransferDebugPair {

llvm/trunk/test/DebugInfo/X86/live-debug-values-expr-conflict.ll

+; RUN: llc %s -mtriple=x86_64-unknown-unknown -o - -stop-after=livedebugvalues | FileCheck %s
+;
+; In the C below, 'baz' is re-assigned with a value that gets salvaged, making
+; it's dbg.value base itself on 'bar', but with a complex expression.
+; LiveDebugValues should recognize that these are different locations, and not
+; propagate a location for 'baz' into the return block.
+;
+; void escape1(int bees);
+; void escape2(int bees);
+; 
+; int foo(int bar) {
+;   int baz = bar;
+;   if (baz == 12) {
+;     escape1(bar);
+;   } else {
+;     baz += 1;
+;     escape2(bar);
+;   }
+; 
+;   return bar;
+; }
+;
+; We should get a plain DBG_VALUE in the entry block, a plain one then complex
+; one in the block two, and none in block three.
+; CHECK:       ![[BAZVAR:[0-9]+]] = !DILocalVariable(name: "baz",
+; CHECK-LABEL: bb.0.entry:
+; CHECK:       DBG_VALUE {{[0-9a-zA-Z$%_]*}}, $noreg, ![[BAZVAR]],
+; CHECK-SAME:     !DIExpression()
+; CHECK-LABEL: bb.1.if.then:
+; CHECK-LABEL: bb.2.if.else:
+; CHECK:       DBG_VALUE {{[0-9a-zA-Z$%_]*}}, $noreg, ![[BAZVAR]],
+; CHECK-SAME:     !DIExpression()
+; CHECK:       DBG_VALUE {{[0-9a-zA-Z$%_]*}}, $noreg, ![[BAZVAR]],
+; CHECK-SAME:     !DIExpression(DW_OP_plus_uconst, 1, DW_OP_stack_value)
+; CHECK-LABEL: bb.3.if.end:
+; CHECK-NOT:   DBG_VALUE
+
+declare void @escape1(i32)
+declare void @escape2(i32)
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+define i32 @foo(i32 returned %bar) !dbg !7 {
+entry:
+  call void @llvm.dbg.value(metadata i32 %bar, metadata !13, metadata !DIExpression()), !dbg !14
+  %cmp = icmp eq i32 %bar, 12, !dbg !14
+  br i1 %cmp, label %if.then, label %if.else, !dbg !14
+
+if.then:
+  tail call void @escape1(i32 12) #3, !dbg !14
+  br label %if.end, !dbg !14
+
+if.else:
+  call void @llvm.dbg.value(metadata i32 %bar, metadata !13, metadata !DIExpression(DW_OP_plus_uconst, 1, DW_OP_stack_value)), !dbg !14
+  tail call void @escape2(i32 %bar) #3, !dbg !14
+  br label %if.end
+
+if.end:
+  ret i32 %bar, !dbg !14
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "exprconflict.c", directory: "/home/jmorse")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 5, type: !8, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!13}
+!13 = !DILocalVariable(name: "baz", scope: !7, file: !1, line: 6, type: !10)
+!14 = !DILocation(line: 1, scope: !7)