[DebugInfo] Teach LDV how to handle identical variable fragments
authorOCHyams <orlando.hyams@sony.com>
Tue, 11 Feb 2020 09:44:32 +0000 (09:44 +0000)
committerOCHyams <orlando.hyams@sony.com>
Tue, 11 Feb 2020 10:20:24 +0000 (10:20 +0000)
LiveDebugVariables uses interval maps to explicitly represent DBG_VALUE
intervals. DBG_VALUEs are filtered into an interval map based on their {
Variable, DIExpression }. The interval map will coalesce adjacent entries that
use the same { Location }.  Under this model, DBG_VALUEs which refer to the same
bits of the same variable will be filtered into different interval maps if they
have different DIExpressions which means the original intervals will not be
properly preserved.

This patch fixes the problem by using { Variable, Fragment } to filter the
DBG_VALUEs into maps, and coalesces adjacent entries iff they have the same
{ Location, DIExpression } pair.

The solution is not perfect because we see the similar issues appear when
partially overlapping fragments are encountered, but is far simpler than a
complete solution (i.e. D70121).

Fixes: pr41992, pr43957
Reviewed By: aprantl
Differential Revision: https://reviews.llvm.org/D74053

llvm/lib/CodeGen/LiveDebugVariables.cpp
llvm/test/DebugInfo/X86/live-debug-vars-intervals.mir [new file with mode: 0644]
llvm/test/tools/llvm-locstats/locstats.ll

index 5b20a24..8712c6e 100644 (file)
@@ -96,18 +96,19 @@ LiveDebugVariables::LiveDebugVariables() : MachineFunctionPass(ID) {
 
 enum : unsigned { UndefLocNo = ~0U };
 
-/// Describes a location by number along with some flags about the original
-/// usage of the location.
+/// Describes a debug variable value by location number and expression along
+/// with some flags about the original usage of the location.
 class DbgValueLocation {
 public:
-  DbgValueLocation(unsigned LocNo, bool WasIndirect)
-      : LocNo(LocNo), WasIndirect(WasIndirect) {
-    static_assert(sizeof(*this) == sizeof(unsigned), "bad bitfield packing");
+  DbgValueLocation(unsigned LocNo, bool WasIndirect,
+                   const DIExpression &Expression)
+      : LocNo(LocNo), WasIndirect(WasIndirect), Expression(&Expression) {
     assert(locNo() == LocNo && "location truncation");
   }
 
   DbgValueLocation() : LocNo(0), WasIndirect(0) {}
 
+  const DIExpression *getExpression() const { return Expression; }
   unsigned locNo() const {
     // Fix up the undef location number, which gets truncated.
     return LocNo == INT_MAX ? UndefLocNo : LocNo;
@@ -116,12 +117,13 @@ public:
   bool isUndef() const { return locNo() == UndefLocNo; }
 
   DbgValueLocation changeLocNo(unsigned NewLocNo) const {
-    return DbgValueLocation(NewLocNo, WasIndirect);
+    return DbgValueLocation(NewLocNo, WasIndirect, *Expression);
   }
 
   friend inline bool operator==(const DbgValueLocation &LHS,
                                 const DbgValueLocation &RHS) {
-    return LHS.LocNo == RHS.LocNo && LHS.WasIndirect == RHS.WasIndirect;
+    return LHS.LocNo == RHS.LocNo && LHS.WasIndirect == RHS.WasIndirect &&
+           LHS.Expression == RHS.Expression;
   }
 
   friend inline bool operator!=(const DbgValueLocation &LHS,
@@ -132,6 +134,7 @@ public:
 private:
   unsigned LocNo : 31;
   unsigned WasIndirect : 1;
+  const DIExpression *Expression = nullptr;
 };
 
 /// Map of where a user value is live, and its location.
@@ -156,6 +159,8 @@ class LDVImpl;
 /// closure of that relation.
 class UserValue {
   const DILocalVariable *Variable; ///< The debug info variable we are part of.
+  // FIXME: This is only used to get the FragmentInfo that describes the part
+  // of the variable we are a part of. We should just store the FragmentInfo.
   const DIExpression *Expression; ///< Any complex address expression.
   DebugLoc dl;            ///< The debug location for the variable. This is
                           ///< used by dwarf writer to find lexical scope.
@@ -205,9 +210,19 @@ public:
   /// Does this UserValue match the parameters?
   bool match(const DILocalVariable *Var, const DIExpression *Expr,
              const DILocation *IA) const {
-    // FIXME: The fragment should be part of the equivalence class, but not
-    // other things in the expression like stack values.
-    return Var == Variable && Expr == Expression && dl->getInlinedAt() == IA;
+    // FIXME: Handle partially overlapping fragments.
+    // A DBG_VALUE with a fragment which overlaps a previous DBG_VALUE fragment
+    // for the same variable terminates the interval opened by the first.
+    // getUserValue() uses match() to filter DBG_VALUEs into interval maps to
+    // represent these intervals.
+    // Given two _partially_ overlapping fragments match() will always return
+    // false. The DBG_VALUEs will be filtered into separate interval maps and
+    // therefore we do not faithfully represent the original intervals.
+    // See D70121#1849741 for a more detailed explanation and further
+    // discussion.
+    return Var == Variable &&
+           Expr->getFragmentInfo() == Expression->getFragmentInfo() &&
+           dl->getInlinedAt() == IA;
   }
 
   /// Merge equivalence classes.
@@ -285,8 +300,9 @@ public:
   void mapVirtRegs(LDVImpl *LDV);
 
   /// Add a definition point to this value.
-  void addDef(SlotIndex Idx, const MachineOperand &LocMO, bool IsIndirect) {
-    DbgValueLocation Loc(getLocationNo(LocMO), IsIndirect);
+  void addDef(SlotIndex Idx, const MachineOperand &LocMO, bool IsIndirect,
+              const DIExpression &Expr) {
+    DbgValueLocation Loc(getLocationNo(LocMO), IsIndirect, Expr);
     // Add a singular (Idx,Idx) -> Loc mapping.
     LocMap::iterator I = locInts.find(Idx);
     if (!I.valid() || I.start() != Idx)
@@ -320,12 +336,11 @@ public:
   /// possible.
   ///
   /// \param LI Scan for copies of the value in LI->reg.
-  /// \param LocNo Location number of LI->reg.
-  /// \param WasIndirect Indicates if the original use of LI->reg was indirect
+  /// \param Loc Location number of LI->reg.
   /// \param Kills Points where the range of LocNo could be extended.
   /// \param [in,out] NewDefs Append (Idx, LocNo) of inserted defs here.
   void addDefsFromCopies(
-      LiveInterval *LI, unsigned LocNo, bool WasIndirect,
+      LiveInterval *LI, DbgValueLocation Loc,
       const SmallVectorImpl<SlotIndex> &Kills,
       SmallVectorImpl<std::pair<SlotIndex, DbgValueLocation>> &NewDefs,
       MachineRegisterInfo &MRI, LiveIntervals &LIS);
@@ -663,11 +678,11 @@ bool LDVImpl::handleDebugValue(MachineInstr &MI, SlotIndex Idx) {
   UserValue *UV =
       getUserValue(Var, Expr, MI.getDebugLoc());
   if (!Discard)
-    UV->addDef(Idx, MI.getOperand(0), IsIndirect);
+    UV->addDef(Idx, MI.getOperand(0), IsIndirect, *Expr);
   else {
     MachineOperand MO = MachineOperand::CreateReg(0U, false);
     MO.setIsDebug();
-    UV->addDef(Idx, MO, false);
+    UV->addDef(Idx, MO, false, *Expr);
   }
   return true;
 }
@@ -775,7 +790,7 @@ void UserValue::extendDef(SlotIndex Idx, DbgValueLocation Loc, LiveRange *LR,
 }
 
 void UserValue::addDefsFromCopies(
-    LiveInterval *LI, unsigned LocNo, bool WasIndirect,
+    LiveInterval *LI, DbgValueLocation Loc,
     const SmallVectorImpl<SlotIndex> &Kills,
     SmallVectorImpl<std::pair<SlotIndex, DbgValueLocation>> &NewDefs,
     MachineRegisterInfo &MRI, LiveIntervals &LIS) {
@@ -805,7 +820,7 @@ void UserValue::addDefsFromCopies(
     // it, or we are looking at a wrong value of LI.
     SlotIndex Idx = LIS.getInstructionIndex(*MI);
     LocMap::iterator I = locInts.find(Idx.getRegSlot(true));
-    if (!I.valid() || I.value().locNo() != LocNo)
+    if (!I.valid() || I.value() != Loc)
       continue;
 
     if (!LIS.hasInterval(DstReg))
@@ -839,7 +854,7 @@ void UserValue::addDefsFromCopies(
       MachineInstr *CopyMI = LIS.getInstructionFromIndex(DstVNI->def);
       assert(CopyMI && CopyMI->isCopy() && "Bad copy value");
       unsigned LocNo = getLocationNo(CopyMI->getOperand(0));
-      DbgValueLocation NewLoc(LocNo, WasIndirect);
+      DbgValueLocation NewLoc = Loc.changeLocNo(LocNo);
       I.insert(Idx, Idx.getNextSlot(), NewLoc);
       NewDefs.push_back(std::make_pair(Idx, NewLoc));
       break;
@@ -887,8 +902,7 @@ void UserValue::computeIntervals(MachineRegisterInfo &MRI,
       // sub-register in that regclass). For now, simply skip handling copies if
       // a sub-register is involved.
       if (LI && !LocMO.getSubReg())
-        addDefsFromCopies(LI, Loc.locNo(), Loc.wasIndirect(), Kills, Defs, MRI,
-                          LIS);
+        addDefsFromCopies(LI, Loc, Kills, Defs, MRI, LIS);
       continue;
     }
 
@@ -1329,7 +1343,7 @@ void UserValue::insertDebugValue(MachineBasicBlock *MBB, SlotIndex StartIdx,
   // original DBG_VALUE was indirect, we need to add DW_OP_deref to indicate
   // that the original virtual register was a pointer. Also, add the stack slot
   // offset for the spilled register to the expression.
-  const DIExpression *Expr = Expression;
+  const DIExpression *Expr = Loc.getExpression();
   uint8_t DIExprFlags = DIExpression::ApplyOffset;
   bool IsIndirect = Loc.wasIndirect();
   if (Spilled) {
diff --git a/llvm/test/DebugInfo/X86/live-debug-vars-intervals.mir b/llvm/test/DebugInfo/X86/live-debug-vars-intervals.mir
new file mode 100644 (file)
index 0000000..f215b50
--- /dev/null
@@ -0,0 +1,154 @@
+# These tests check that DBG_VALUE intervals are correctly coalesced (or not) in
+# LiveDebugVariables.
+# NOTE: We do not currently handle partially overlapping fragments in LDV.
+#
+# The IR in this file has been modified by hand, generated from this source:
+# void escape(int *);
+# extern int global;
+# void f(int x) {
+#   escape(&x);
+#   x = 1;
+#   global = x;
+#   x = 2;
+#   escape(&x);
+# }
+
+# RUN: llc %s -start-before=machine-scheduler -stop-after=virtregrewriter -o - \
+# RUN:     | FileCheck %s --implicit-check-not=DBG_VALUE
+
+# Verify that DBG_VALUEs with same { Variable, Fragment } but different DIExpressions
+# are not coalesced.
+#
+# DV1 %0 "x" (deref) --+-- Do not coalesce these because DV2 refers to the same bits.
+# DV2 %0 "x" ()        |
+# DV3 %0 "x" (deref) --+
+#
+# CHECK: name: f1
+# CHECK: DBG_VALUE %stack.0.x.addr, $noreg, ![[VAR:[0-9]+]], !DIExpression(DW_OP_deref)
+# CHECK: DBG_VALUE %stack.0.x.addr, $noreg, ![[VAR]],        !DIExpression()
+# CHECK: DBG_VALUE %stack.0.x.addr, $noreg, ![[VAR]],        !DIExpression(DW_OP_deref)
+
+# Verify that DBG_VALUEs with same { Variable, Fragment } and { Location, DIExpression } are
+# coalesced if there are no intervening DBG_VALUEs with the same { Variable, Fragment }.
+#
+# DV1 %0 "x" (fragment 00 16) --+-- Coalesce these because DV2 refers to different bits.
+# DV2 %0 "x" (fragment 16 16)   |
+# DV3 %0 "x" (fragment 00 16) --+
+#
+# CHECK: name: f2
+# CHECK: DBG_VALUE %stack.0.x.addr, $noreg, ![[VAR:[0-9]+]], !DIExpression(DW_OP_deref, DW_OP_LLVM_fragment, 0, 16)
+# CHECK: DBG_VALUE %stack.0.x.addr, $noreg, ![[VAR]],        !DIExpression(DW_OP_LLVM_fragment, 16, 16)
+
+--- |
+  target triple = "x86_64-unknown-linux-gnu"
+
+  @global = external global i32, align 4
+  declare void @llvm.dbg.value(metadata, metadata, metadata)
+  declare void @escape(i32*)
+
+  define void @f1(i32 %x) !dbg !6 {
+  entry:
+    %x.addr = alloca i32, align 4
+    store i32 %x, i32* %x.addr, align 4
+    call void @llvm.dbg.value(metadata i32* %x.addr, metadata !11, metadata !DIExpression(DW_OP_deref)), !dbg !12
+    call void @escape(i32* %x.addr), !dbg !12
+    call void @llvm.dbg.value(metadata i32* %x.addr, metadata !11, metadata !DIExpression()), !dbg !12
+    store i32 1, i32* @global, align 4, !dbg !12
+    call void @llvm.dbg.value(metadata i32* %x.addr, metadata !11, metadata !DIExpression(DW_OP_deref)), !dbg !12
+    store i32 2, i32* %x.addr, align 4, !dbg !12
+    call void @escape(i32* %x.addr), !dbg !12
+    ret void, !dbg !12
+  }
+
+  define void @f2(i32 %x) !dbg !13 {
+  entry:
+    %x.addr = alloca i32, align 4
+    store i32 %x, i32* %x.addr, align 4
+    call void @llvm.dbg.value(metadata i32* %x.addr, metadata !14, metadata !DIExpression(DW_OP_deref, DW_OP_LLVM_fragment, 0, 16)), !dbg !15
+    call void @escape(i32* %x.addr)
+    call void @llvm.dbg.value(metadata i32* %x.addr, metadata !14, metadata !DIExpression(DW_OP_LLVM_fragment, 16, 16)), !dbg !15
+    store i32 1, i32* @global, align 4
+    call void @llvm.dbg.value(metadata i32* %x.addr, metadata !14, metadata !DIExpression(DW_OP_deref, DW_OP_LLVM_fragment, 0, 16)), !dbg !15
+    store i32 2, i32* %x.addr, align 4
+    call void @escape(i32* %x.addr)
+    ret void
+  }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4}
+  !llvm.ident = !{!5}
+
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0 ", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+  !1 = !DIFile(filename: "test.c", directory: "")
+  !2 = !{}
+  !3 = !{i32 2, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{!"clang version 11.0.0 "}
+  !6 = distinct !DISubprogram(name: "f1", scope: !1, file: !1, line: 3, type: !7, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !10)
+  !7 = !DISubroutineType(types: !8)
+  !8 = !{null, !9}
+  !9 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !10 = !{!11}
+  !11 = !DILocalVariable(name: "x", arg: 1, scope: !6, file: !1, line: 3, type: !9)
+  !12 = !DILocation(line: 3, column: 12, scope: !6)
+  !13 = distinct !DISubprogram(name: "f2", scope: !1, file: !1, line: 20, type: !7, scopeLine: 20, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !10)
+  !14 = !DILocalVariable(name: "x", arg: 1, scope: !13, file: !1, line: 21, type: !9)
+  !15 = !DILocation(line: 23, column: 12, scope: !13)
+
+...
+---
+name:            f1
+tracksRegLiveness: true
+stack:
+  - { id: 0, name: x.addr, type: default, offset: 0, size: 4, alignment: 4,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+body:             |
+  bb.0.entry:
+    liveins: $edi
+    %0:gr32 = COPY $edi
+    MOV32mr %stack.0.x.addr, 1, $noreg, 0, $noreg, %0 :: (store 4 into %ir.x.addr)
+    DBG_VALUE %stack.0.x.addr, $noreg, !11, !DIExpression(DW_OP_deref), debug-location !12
+    ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp, debug-location !12
+    %1:gr64 = LEA64r %stack.0.x.addr, 1, $noreg, 0, $noreg
+    $rdi = COPY %1, debug-location !12
+    CALL64pcrel32 @escape, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp, debug-location !12
+    ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp, debug-location !12
+    DBG_VALUE %stack.0.x.addr, $noreg, !11, !DIExpression(), debug-location !12
+    MOV32mi $rip, 1, $noreg, @global, $noreg, 1, debug-location !12 :: (store 4 into @global)
+    DBG_VALUE %stack.0.x.addr, $noreg, !11, !DIExpression(DW_OP_deref), debug-location !12
+    MOV32mi %stack.0.x.addr, 1, $noreg, 0, $noreg, 2, debug-location !12 :: (store 4 into %ir.x.addr)
+    ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp, debug-location !12
+    $rdi = COPY %1, debug-location !12
+    CALL64pcrel32 @escape, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp, debug-location !12
+    ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp, debug-location !12
+    RET 0, debug-location !12
+...
+---
+name:            f2
+tracksRegLiveness: true
+stack:
+  - { id: 0, name: x.addr, type: default, offset: 0, size: 4, alignment: 4,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+body:             |
+  bb.0.entry:
+    liveins: $edi
+    %0:gr32 = COPY $edi
+    MOV32mr %stack.0.x.addr, 1, $noreg, 0, $noreg, %0 :: (store 4 into %ir.x.addr)
+    DBG_VALUE %stack.0.x.addr, $noreg, !14, !DIExpression(DW_OP_deref, DW_OP_LLVM_fragment, 0, 16), debug-location !15
+    ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    %1:gr64 = LEA64r %stack.0.x.addr, 1, $noreg, 0, $noreg
+    $rdi = COPY %1
+    CALL64pcrel32 @escape, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp
+    ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    DBG_VALUE %stack.0.x.addr, $noreg, !14, !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !15
+    MOV32mi $rip, 1, $noreg, @global, $noreg, 1 :: (store 4 into @global)
+    DBG_VALUE %stack.0.x.addr, $noreg, !14, !DIExpression(DW_OP_deref, DW_OP_LLVM_fragment, 0, 16), debug-location !15
+    MOV32mi %stack.0.x.addr, 1, $noreg, 0, $noreg, 2 :: (store 4 into %ir.x.addr)
+    ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    $rdi = COPY %1
+    CALL64pcrel32 @escape, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp
+    ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    RET 0
+...
index e267b7e..f16635d 100644 (file)
@@ -11,9 +11,9 @@
 ; LOCSTATS: [30%,40%) 0 0%
 ; LOCSTATS: [40%,50%) 1 11%
 ; LOCSTATS: [50%,60%) 1 11%
-; LOCSTATS: [60%,70%) 0 0%
+; LOCSTATS: [60%,70%) 1 11%
 ; LOCSTATS: [70%,80%) 0 0%
-; LOCSTATS: [80%,90%) 3 33%
+; LOCSTATS: [80%,90%) 2 22%
 ; LOCSTATS: [90%,100%) 1 11%
 ; LOCSTATS: 100% 2 22%
 ;