[DwarfDebug] Improve single location detection in validThroughout (2/4)

With this patch we're now accounting for two more cases which should be
considered 'valid throughout': First, where RangeEnd is ScopeEnd. Second, where
RangeEnd comes before ScopeEnd when including meta instructions, but are both
preceded by the same non-meta instruction.

CTMark shows a geomean binary size reduction of 1.5% for RelWithDebInfo builds.
`llvm-locstats` (using D85636) shows a very small variable location coverage
change in 2 of 10 binaries, but it is in the order of 10s of bytes which lines
up with my expectations.

I've added a test which checks both of these new cases. The first check in the
test isn't strictly necessary for this patch. But I'm not sure that it is
explicitly tested anywhere else, and is useful for the final patch in the
series.

Reviewed By: aprantl

Differential Revision: https://reviews.llvm.org/D86151

llvm/include/llvm/CodeGen/DbgEntityHistoryCalculator.h

@@ -24,6 +24,24 @@ class MachineFunction;
 class MachineInstr;
 class TargetRegisterInfo;
 
+/// Record instruction ordering so we can query their relative positions within
+/// a function. Meta instructions are given the same ordinal as the preceding
+/// non-meta instruction. Class state is invalid if MF is modified after
+/// calling initialize.
+class InstructionOrdering {
+public:
+  void initialize(const MachineFunction &MF);
+  void clear() { InstNumberMap.clear(); }
+
+  /// Check if instruction \p A comes before \p B, where \p A and \p B both
+  /// belong to the MachineFunction passed to initialize().
+  bool isBefore(const MachineInstr *A, const MachineInstr *B) const;
+
+private:
+  /// Each instruction is assigned an order number.
+  DenseMap<const MachineInstr *, unsigned> InstNumberMap;
+};
+
 /// For each user variable, keep a list of instruction ranges where this
 /// variable is accessible. The variables are listed in order of appearance.
 class DbgValueHistoryMap {
@@ -93,7 +111,8 @@ public:
   }
 
   /// Drop location ranges which exist entirely outside each variable's scope.
-  void trimLocationRanges(const MachineFunction &MF, LexicalScopes &LScopes);
+  void trimLocationRanges(const MachineFunction &MF, LexicalScopes &LScopes,
+                          const InstructionOrdering &Ordering);
   bool empty() const { return VarEntries.empty(); }
   void clear() { VarEntries.clear(); }
   EntriesMap::const_iterator begin() const { return VarEntries.begin(); }

llvm/include/llvm/CodeGen/DebugHandlerBase.h

@@ -110,6 +110,9 @@ protected:
   virtual void endFunctionImpl(const MachineFunction *MF) = 0;
   virtual void skippedNonDebugFunction() {}
 
+private:
+  InstructionOrdering InstOrdering;
+
   // AsmPrinterHandler overrides.
 public:
   void beginInstruction(const MachineInstr *MI) override;
@@ -129,8 +132,10 @@ public:
 
   /// If this type is derived from a base type then return base type size.
   static uint64_t getBaseTypeSize(const DIType *Ty);
+
+  const InstructionOrdering &getInstOrdering() const { return InstOrdering; }
 };
 
-}
+} // namespace llvm
 
 #endif

llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

@@ -53,24 +53,6 @@ static Register isDescribedByReg(const MachineInstr &MI) {
                                        : Register();
 }
 
-/// Record instruction ordering so we can query their relative positions within
-/// a function. Meta instructions are given the same ordinal as the preceding
-/// non-meta instruction. Class state is invalid if MF is modified after
-/// calling initialize.
-class InstructionOrdering {
-public:
-  void initialize(const MachineFunction &MF);
-  void clear() { InstNumberMap.clear(); }
-
-  /// Check if instruction \p A comes before \p B, where \p A and \p B both
-  /// belong to the MachineFunction passed to initialize().
-  bool isBefore(const MachineInstr *A, const MachineInstr *B) const;
-
-private:
-  /// Each instruction is assigned an order number.
-  DenseMap<const MachineInstr *, unsigned> InstNumberMap;
-};
-
 void InstructionOrdering::initialize(const MachineFunction &MF) {
   // We give meta instructions the same ordinal as the preceding instruction
   // because this class is written for the task of comparing positions of
@@ -161,11 +143,9 @@ intersects(const MachineInstr *StartMI, const MachineInstr *EndMI,
   return None;
 }
 
-void DbgValueHistoryMap::trimLocationRanges(const MachineFunction &MF,
-                                            LexicalScopes &LScopes) {
-  InstructionOrdering Ordering;
-  Ordering.initialize(MF);
-
+void DbgValueHistoryMap::trimLocationRanges(
+    const MachineFunction &MF, LexicalScopes &LScopes,
+    const InstructionOrdering &Ordering) {
   // The indices of the entries we're going to remove for each variable.
   SmallVector<EntryIndex, 4> ToRemove;
   // Entry reference count for each variable. Clobbers left with no references

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

@@ -196,8 +196,9 @@ void DebugHandlerBase::beginFunction(const MachineFunction *MF) {
   assert(DbgLabels.empty() && "DbgLabels map wasn't cleaned!");
   calculateDbgEntityHistory(MF, Asm->MF->getSubtarget().getRegisterInfo(),
                             DbgValues, DbgLabels);
+  InstOrdering.initialize(*MF);
   if (TrimVarLocs)
-    DbgValues.trimLocationRanges(*MF, LScopes);
+    DbgValues.trimLocationRanges(*MF, LScopes, InstOrdering);
   LLVM_DEBUG(DbgValues.dump());
 
   // Request labels for the full history.
@@ -333,6 +334,7 @@ void DebugHandlerBase::endFunction(const MachineFunction *MF) {
   DbgLabels.clear();
   LabelsBeforeInsn.clear();
   LabelsAfterInsn.clear();
+  InstOrdering.clear();
 }
 
 void DebugHandlerBase::beginBasicBlock(const MachineBasicBlock &MBB) {

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

@@ -1518,7 +1518,8 @@ void DwarfDebug::collectVariableInfoFromMFTable(
 /// either open or otherwise rolls off the end of the scope.
 static bool validThroughout(LexicalScopes &LScopes,
                             const MachineInstr *DbgValue,
-                            const MachineInstr *RangeEnd) {
+                            const MachineInstr *RangeEnd,
+                            const InstructionOrdering &Ordering) {
   assert(DbgValue->getDebugLoc() && "DBG_VALUE without a debug location");
   auto MBB = DbgValue->getParent();
   auto DL = DbgValue->getDebugLoc();
@@ -1586,9 +1587,8 @@ static bool validThroughout(LexicalScopes &LScopes,
 
   // The location range and variable's enclosing scope are both contained within
   // MBB, test if location terminates before end of scope.
-  for (auto I = RangeEnd->getIterator(); I != MBB->end(); ++I)
-    if (&*I == LScopeEnd)
-      return false;
+  if (Ordering.isBefore(RangeEnd, LScopeEnd))
+    return false;
 
   // There's a single location which starts at the scope start, and ends at or
   // after the scope end.
@@ -1734,7 +1734,7 @@ bool DwarfDebug::buildLocationList(
     return false;
   if (VeryLargeBlocks.count(StartDebugMI->getParent()))
     return false;
-  return validThroughout(LScopes, StartDebugMI, EndMI);
+  return validThroughout(LScopes, StartDebugMI, EndMI, getInstOrdering());
 }
 
 DbgEntity *DwarfDebug::createConcreteEntity(DwarfCompileUnit &TheCU,
@@ -1810,7 +1810,7 @@ void DwarfDebug::collectEntityInfo(DwarfCompileUnit &TheCU,
       const auto *End =
           SingleValueWithClobber ? HistoryMapEntries[1].getInstr() : nullptr;
       if (VeryLargeBlocks.count(MInsn->getParent()) == 0 &&
-          validThroughout(LScopes, MInsn, End)) {
+          validThroughout(LScopes, MInsn, End, getInstOrdering())) {
         RegVar->initializeDbgValue(MInsn);
         continue;
       }

llvm/test/DebugInfo/MIR/AArch64/implicit-def-dead-scope.mir

@@ -5,9 +5,8 @@
 #  encountering an IMPLICIT_DEF in its own lexical scope.
 
 # CHECK: .debug_info contents:
-# CHECK:       DW_TAG_formal_parameter
-# CHECK:       DW_AT_location [DW_FORM_sec_offset]
-# CHECK-NEXT:                 DW_OP_lit0, DW_OP_stack_value
+# CHECK:       DW_TAG_formal_parameter [14]
+# CHECK-NEXT:  DW_AT_const_value [DW_FORM_udata]    (0)
 # CHECK-NEXT:  DW_AT_abstract_origin {{.*}} "name"
 --- |
   ; ModuleID = 't.ll'

llvm/test/DebugInfo/MIR/X86/callsite-stack-value.mir

@@ -2,8 +2,7 @@
 # RUN:   -emit-call-site-info | llvm-dwarfdump - | FileCheck %s -implicit-check-not=call_site_parameter
 
 # CHECK: DW_TAG_formal_parameter
-# CHECK-NEXT: DW_AT_location
-# CHECK-NEXT: DW_OP_reg17
+# CHECK-NEXT: DW_AT_location (DW_OP_reg17 XMM0)
 
 # struct S {
 #   float w;

llvm/test/DebugInfo/X86/single-location-2.mir

@@ -0,0 +1,92 @@
+# RUN: llc %s --start-after=livedebugvalues -filetype=obj -o - \
+# RUN:     | llvm-dwarfdump - -name local* -regex \
+# RUN:     | FileCheck %s
+#
+## This tests certain single location detection functionality. The Test MIR
+## is hand written. Test directives and comments inline.
+
+--- |
+  target triple = "x86_64-unknown-linux-gnu"
+  define dso_local i32 @fun() local_unnamed_addr !dbg !7 {
+  entry:
+    ret i32 0
+  }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4, !5}
+  !llvm.ident = !{!6}
+
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "example.c", directory: "/")
+  !3 = !{i32 7, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 4}
+  !6 = !{!"clang version 11.0.0"}
+  !8 = !DISubroutineType(types: !9)
+  !9 = !{!10}
+  !10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !22 = !DISubroutineType(types: !23)
+  !23 = !{!10, !10}
+  ; --- Important metadata ---
+  !7 = distinct !DISubprogram(name: "fun", scope: !1, file: !1, line: 2, type: !8, scopeLine: 2, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+  !24 = distinct !DILexicalBlock(scope: !7, file: !1, line: 9, column: 3)
+  !14 = distinct !DILexicalBlock(scope: !7, file: !1, line: 4, column: 3)
+  !12 = !DILocalVariable(name: "locala", scope: !7, file: !1, line: 1, type: !10)
+  !13 = !DILocalVariable(name: "localb", scope: !14, file: !1, line: 2, type: !10)
+  !25 = !DILocalVariable(name: "localc", scope: !24, file: !1, line: 3, type: !10)
+  !27 = !DILocalVariable(name: "tmp",    scope: !14, file: !1, line: 2, type: !10)
+  !15 = !DILocation(line: 1, column: 0, scope: !7)
+  !18 = !DILocation(line: 2, column: 1, scope: !14)
+  !26 = !DILocation(line: 3, column: 1, scope: !24)
+...
+---
+name:            fun
+body:             |
+  bb.0.entry:
+    ;; This is the scope and variable structure:
+    ;; int fun() {       // scope fun !7
+    ;;   int locala;     // scope fun !7,        var locala !12, debug-location !15
+    ;;   { int localb;   // scope fun:block !14, var localb !13, debug-location !18
+    ;;     int tmp;    } // scope fun:block !14, var localb !27, debug-location !18
+    ;;   { int localc; } // scope fun:block !24, var localc !25, debug-location !26
+    ;; }
+    ;;
+    ;; (1) Check that frame-setup instructions are not counted against
+    ;;     locations being valid throughout the function call.
+    ;
+    ; CHECK:      DW_TAG_variable
+    ; CHECK-NEXT:   DW_AT_location (DW_OP_reg5 RDI)
+    ; CHECK-NEXT:   DW_AT_name ("locala")
+    $rbp = frame-setup MOV64rr $rsp
+    DBG_VALUE $edi, $noreg, !12, !DIExpression(), debug-location !15
+    $eax = MOV32ri 0, debug-location !15
+
+    ;; (2) The scope block ends with a meta instruction. A location range ends
+    ;;     with the final non-meta instruction in the scope. Check that
+    ;;     location is considered valid throughout.
+    ;
+    ; CHECK:      DW_TAG_variable
+    ; CHECK-NEXT:   DW_AT_location (DW_OP_reg2 RCX)
+    ; CHECK-NEXT:   DW_AT_name ("localb")
+    ;
+    ;; start scope, start location range
+    DBG_VALUE $ecx, $noreg, !13, !DIExpression(), debug-location !18
+    ;; end location range
+    $ecx = MOV32ri 1, debug-location !18
+    ;; end scope
+    DBG_VALUE $noreg, $noreg, !27, !DIExpression(), debug-location !18
+
+    ;; (3) The final instruction in the scope closes a location range. Check
+    ;;     that location is considered valid throughout.
+    ;
+    ; CHECK:      DW_TAG_variable
+    ; CHECK-NEXT:   DW_AT_location (DW_OP_reg4 RSI)
+    ; CHECK-NEXT:   DW_AT_name ("localc")
+    ;
+    ;; start scope, start location range
+    DBG_VALUE $esi, $noreg, !25, !DIExpression(), debug-location !26
+    ;; end scope, end location range
+    $esi = MOV32ri 2, debug-location !26
+
+    RETQ debug-location !15
+...

llvm/test/DebugInfo/X86/trim-var-locs.mir

@@ -81,8 +81,7 @@ body:             |
     ;     block is not trimmed.
     ;
     ; CHECK:      DW_TAG_variable
-    ; CHECK-NEXT:   DW_AT_location
-    ; CHECK-NEXT:     DW_OP_reg5 RDI
+    ; CHECK-NEXT:   DW_AT_location (DW_OP_reg5 RDI)
     ; CHECK-NEXT:   DW_AT_name ("localb")
     ;
     ; localb range 2 clobber in scope fun !7 (outside block !14)