Propagate DBG_VALUE entries when there are unvisited predecessors

[keith.walker.arm]

Summary

Variables are sometimes missing their debug location information in
blocks in which the variables should be available. This would occur
when one or more predecessor blocks had not yet been visited by the
routine which propagated the information from predecessor blocks.

This is addressed by only considering predecessor blocks which have
already been visited.

The solution to this problem was suggested by Daniel Berlin on the
LLVM developer mailing list.

[• dberlin]

LGTM algorithmically, i'll let Adrian give the final okay here.

[aprantl]

Generally looks good to me.
The testcase appears to be a bit fragile — it might make sense to convert this to a MIR testcase so we aren't depending on random codegen choices.

lib/CodeGen/LiveDebugValues.cpp

384

reverse

test/CodeGen/ARM/dbg-range-extension.ll

3 ↗

(On Diff #72507)

propagated

[probinson]

lib/CodeGen/LiveDebugValues.cpp

385

considered

[keith.walker.arm]

Correct spelling mistakes in comments.
Converted the test to a MIR testcase

[aprantl]

Thanks! Please watch the compile-time performance bots after committing this. This could cause many more DBG_VALUE instructions to be inserted and could cause a regression. Contrary to the old LiveDebugVariables pass we currently don't look at the lexical scopes before inserting a DBG_VALUE intrinsic which means that we often propagate them much further down than necessary. This is especially noticeable in large C++ functions with many inlined method calls that all use the same "this"-pointer. I mean to fix this soon but haven't gotten around to it yet.

lib/CodeGen/LiveDebugValues.cpp

private:
   void transferRegisterDef(MachineInstr &MI, OpenRangesSet &OpenRanges,
                            const VarLocMap &VarLocIDs);
   bool transferTerminatorInst(MachineInstr &MI, OpenRangesSet &OpenRanges,
                               VarLocInMBB &OutLocs, const VarLocMap &VarLocIDs);
   bool transfer(MachineInstr &MI, OpenRangesSet &OpenRanges,
                 VarLocInMBB &OutLocs, VarLocMap &VarLocIDs);
 
   bool join(MachineBasicBlock &MBB, VarLocInMBB &OutLocs, VarLocInMBB &InLocs,
-            const VarLocMap &VarLocIDs);
+            const VarLocMap &VarLocIDs,
+            SmallPtrSet<const MachineBasicBlock *, 16> &Visited);
 
   bool ExtendRanges(MachineFunction &MF);
 
 public:
   static char ID;
 
   /// Default construct and initialize the pass.
   LiveDebugValues();
bool LiveDebugValues::transfer(MachineInstr &MI, OpenRangesSet &OpenRanges,
   Changed = transferTerminatorInst(MI, OpenRanges, OutLocs, VarLocIDs);
   return Changed;
 }
 
 /// This routine joins the analysis results of all incoming edges in @MBB by
 /// inserting a new DBG_VALUE instruction at the start of the @MBB - if the same
 /// source variable in all the predecessors of @MBB reside in the same location.
 bool LiveDebugValues::join(MachineBasicBlock &MBB, VarLocInMBB &OutLocs,
-                           VarLocInMBB &InLocs, const VarLocMap &VarLocIDs) {
+                           VarLocInMBB &InLocs, const VarLocMap &VarLocIDs,
+                           SmallPtrSet<const MachineBasicBlock *, 16> &Visited) {
   DEBUG(dbgs() << "join MBB: " << MBB.getName() << "\n");
   bool Changed = false;
 
   VarLocSet InLocsT; // Temporary incoming locations.
 
   // For all predecessors of this MBB, find the set of VarLocs that
   // can be joined.
+  int NumVisited = 0;
   for (auto p : MBB.predecessors()) {
+    // Ignore unvisited predecessor blocks.  As we are processing
+    // the blocks in reverse post-order any unvisited block can
+    // be considered to not remove any incoming values.
+    if (!Visited.count(p))
+      continue;
     auto OL = OutLocs.find(p);
     // Join is null in case of empty OutLocs from any of the pred.
     if (OL == OutLocs.end())
       return false;
 
-    // Just copy over the Out locs to incoming locs for the first predecessor.
-    if (p == *MBB.pred_begin()) {
+    // Just copy over the Out locs to incoming locs for the first visited
+    // predecessor, and for all other predecessors join the Out locs.
+    if (!NumVisited)
       InLocsT = OL->second;
-      continue;
-    }
-    // Join with this predecessor.
+    else
       InLocsT &= OL->second;
+    NumVisited++;
   }
 
+  // As we are processing blocks in reverse post-order we
+  // should have processed at least one predecessor, unless it
+  // is the entry block which has no predecessor.
+  assert((NumVisited || MBB.pred_empty()) &&
+         "Should have processed at least one predecessor");
   if (InLocsT.empty())
     return false;
 
   VarLocSet &ILS = InLocs[&MBB];
 
   // Insert DBG_VALUE instructions, if not already inserted.
   VarLocSet Diff = InLocsT;
   Diff.intersectWithComplement(ILS);
for (auto RI = RPOT.begin(), RE = RPOT.end(); RI != RE; ++RI) {
     BBToOrder[*RI] = RPONumber;
     Worklist.push(RPONumber);
     ++RPONumber;
   }
   // This is a standard "union of predecessor outs" dataflow problem.
   // To solve it, we perform join() and transfer() using the two worklist method
   // until the ranges converge.
   // Ranges have converged when both worklists are empty.
+  SmallPtrSet<const MachineBasicBlock *, 16> Visited;
   while (!Worklist.empty() || !Pending.empty()) {
     // We track what is on the pending worklist to avoid inserting the same
     // thing twice.  We could avoid this with a custom priority queue, but this
     // is probably not worth it.
     SmallPtrSet<MachineBasicBlock *, 16> OnPending;
     DEBUG(dbgs() << "Processing Worklist\n");
     while (!Worklist.empty()) {
       MachineBasicBlock *MBB = OrderToBB[Worklist.top()];
       Worklist.pop();
-      MBBJoined = join(*MBB, OutLocs, InLocs, VarLocIDs);
-
+      MBBJoined = join(*MBB, OutLocs, InLocs, VarLocIDs, Visited);
+      Visited.insert(MBB);
       if (MBBJoined) {
         MBBJoined = false;
         Changed = true;
         for (auto &MI : *MBB)
           OLChanged |= transfer(MI, OpenRanges, OutLocs, VarLocIDs);
 
         DEBUG(printVarLocInMBB(MF, OutLocs, VarLocIDs,
                                "OutLocs after propagating", dbgs()));

test/CodeGen/ARM/dbg-range-extension.mir

+# RUN: llc -mtriple=arm-eabi -run-pass=livedebugvalues %s -o - | FileCheck %s
+#
+# Check that the debug information for variables are propagated into the correct blocks.
+#
+# Generated from the C source:
+#
+# int func2(int,  int);
+# void func(int a) {
+#   int b = func2(10, 11);
+#   if (a) {
+#     int c = func2(12, 13);
+#     for(int i = 1; i < a; i++) {
+#       func2(i, i+b);
+#     }
+#     func2(b,c);
+#   }
+#   func2(b,a);
+# }
+
+# CHECK: [[VAR_A:![0-9]+]] = !DILocalVariable(name: "a",
+# CHECK: [[VAR_B:![0-9]+]] = !DILocalVariable(name: "b",
+# CHECK: [[VAR_C:![0-9]+]] = !DILocalVariable(name: "c",
+# CHECK: [[VAR_I:![0-9]+]] = !DILocalVariable(name: "i",
+
+# CHECK: bb.0.entry
+# CHECK: DBG_VALUE debug-use %r0, debug-use _, [[VAR_A]]
+# CHECK: DBG_VALUE debug-use [[REG_A:%r[0-9]+]], debug-use _, [[VAR_A]]
+# CHECK: DBG_VALUE debug-use [[REG_B:%r[0-9]+]], debug-use _, [[VAR_B]]
+
+# CHECK: bb.1.if.then
+# CHECK: DBG_VALUE debug-use [[REG_B]], debug-use _, [[VAR_B]]
+# CHECK: DBG_VALUE debug-use [[REG_A]], debug-use _, [[VAR_A]]
+# CHECK: DBG_VALUE debug-use [[REG_C:%r[0-9]+]], debug-use _, [[VAR_C]]
+# CHECK: DBG_VALUE 1, 0, [[VAR_I]]
+
+# CHECK: bb.2.for.body
+# CHECK: DBG_VALUE debug-use [[REG_I:%r[0-9]+]], debug-use _, [[VAR_I]]
+# CHECK: DBG_VALUE debug-use [[REG_C]], debug-use _, [[VAR_C]]
+# CHECK: DBG_VALUE debug-use [[REG_B]], debug-use _, [[VAR_B]]
+# CHECK: DBG_VALUE debug-use [[REG_A]], debug-use _, [[VAR_A]]
+# CHECK: DBG_VALUE debug-use [[REG_I]], debug-use _, [[VAR_I]]
+
+# CHECK: bb.3.for.cond
+# CHECK: DBG_VALUE debug-use [[REG_C]], debug-use _, [[VAR_C]]
+# CHECK: DBG_VALUE debug-use [[REG_B]], debug-use _, [[VAR_B]]
+# CHECK: DBG_VALUE debug-use [[REG_A]], debug-use _, [[VAR_A]]
+# CHECK: DBG_VALUE debug-use [[REG_I]], debug-use _, [[VAR_I]]
+
+# CHECK: bb.4.for.cond.cleanup
+# CHECK: DBG_VALUE debug-use [[REG_I]], debug-use _, [[VAR_I]]
+# CHECK: DBG_VALUE debug-use [[REG_C]], debug-use _, [[VAR_C]]
+# CHECK: DBG_VALUE debug-use [[REG_B]], debug-use _, [[VAR_B]]
+# CHECK: DBG_VALUE debug-use [[REG_A]], debug-use _, [[VAR_A]]
+
+# CHECK: bb.5.if.end
+# CHECK: DBG_VALUE debug-use [[REG_B]], debug-use _, [[VAR_B]]
+# CHECK: DBG_VALUE debug-use [[REG_A]], debug-use _, [[VAR_A]]
+--- |
+  ; ModuleID = '/data/kwalker/work/OpenSource-llvm/llvm/test/CodeGen/ARM/dbg-range-extension.ll'
+  source_filename = "/data/kwalker/work/OpenSource-llvm/llvm/test/CodeGen/ARM/dbg-range-extension.ll"
+  target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
+  target triple = "arm---eabi"
+  
+  ; Function Attrs: minsize nounwind optsize
+  define void @func(i32 %a) local_unnamed_addr #0 !dbg !8 {
+  entry:
+    tail call void @llvm.dbg.value(metadata i32 %a, i64 0, metadata !13, metadata !20), !dbg !21
+    %call = tail call i32 @func2(i32 10, i32 11) #0, !dbg !22
+    tail call void @llvm.dbg.value(metadata i32 %call, i64 0, metadata !14, metadata !20), !dbg !23
+    %tobool = icmp eq i32 %a, 0, !dbg !24
+    br i1 %tobool, label %if.end, label %if.then, !dbg !25
+  
+  if.then:                                          ; preds = %entry
+    %call1 = tail call i32 @func2(i32 12, i32 13) #0, !dbg !26
+    tail call void @llvm.dbg.value(metadata i32 %call1, i64 0, metadata !15, metadata !20), !dbg !27
+    tail call void @llvm.dbg.value(metadata i32 1, i64 0, metadata !18, metadata !20), !dbg !28
+    br label %for.cond, !dbg !29
+  
+  for.cond:                                         ; preds = %for.body, %if.then
+    %i.0 = phi i32 [ 1, %if.then ], [ %inc, %for.body ]
+    tail call void @llvm.dbg.value(metadata i32 %i.0, i64 0, metadata !18, metadata !20), !dbg !28
+    %cmp = icmp slt i32 %i.0, %a, !dbg !30
+    br i1 %cmp, label %for.body, label %for.cond.cleanup, !dbg !33
+  
+  for.cond.cleanup:                                 ; preds = %for.cond
+    %call3 = tail call i32 @func2(i32 %call, i32 %call1) #0, !dbg !34
+    br label %if.end, !dbg !35
+  
+  for.body:                                         ; preds = %for.cond
+    %0 = add i32 %call, %i.0, !dbg !36
+    %call2 = tail call i32 @func2(i32 %i.0, i32 %0) #0, !dbg !36
+    %inc = add nuw nsw i32 %i.0, 1, !dbg !38
+    tail call void @llvm.dbg.value(metadata i32 %inc, i64 0, metadata !18, metadata !20), !dbg !28
+    br label %for.cond, !dbg !40, !llvm.loop !41
+  
+  if.end:                                           ; preds = %for.cond.cleanup, %entry
+    %call4 = tail call i32 @func2(i32 %call, i32 %a) #0, !dbg !43
+    ret void, !dbg !44
+  }
+  
+  ; Function Attrs: minsize optsize
+  declare i32 @func2(i32, i32) local_unnamed_addr #1
+  
+  ; Function Attrs: nounwind readnone
+  declare void @llvm.dbg.value(metadata, i64, metadata, metadata) #2
+  
+  ; Function Attrs: nounwind
+  declare void @llvm.stackprotector(i8*, i8**) #3
+  
+  attributes #0 = { minsize nounwind optsize }
+  attributes #1 = { minsize optsize }
+  attributes #2 = { nounwind readnone }
+  attributes #3 = { nounwind }
+  
+  !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", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+  !1 = !DIFile(filename: "loop.c", directory: "/tmp")
+  !2 = !{}
+  !3 = !{i32 2, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 4}
+  !6 = !{i32 1, !"min_enum_size", i32 4}
+  !7 = !{!"clang version 4.0.0 (http://llvm.org/git/clang.git b8f10df3679b36f51e1de7c4351b82d297825089) (http://llvm.org/git/llvm.git c2a5d16d1e3b8c49f5bbb1ff87a76ac4f88edb89)"}
+  !8 = distinct !DISubprogram(name: "func", scope: !1, file: !1, line: 2, type: !9, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !12)
+  !9 = !DISubroutineType(types: !10)
+  !10 = !{null, !11}
+  !11 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+  !12 = !{!13, !14, !15, !18}
+  !13 = !DILocalVariable(name: "a", arg: 1, scope: !8, file: !1, line: 2, type: !11)
+  !14 = !DILocalVariable(name: "b", scope: !8, file: !1, line: 3, type: !11)
+  !15 = !DILocalVariable(name: "c", scope: !16, file: !1, line: 5, type: !11)
+  !16 = distinct !DILexicalBlock(scope: !17, file: !1, line: 4, column: 9)
+  !17 = distinct !DILexicalBlock(scope: !8, file: !1, line: 4, column: 6)
+  !18 = !DILocalVariable(name: "i", scope: !19, file: !1, line: 6, type: !11)
+  !19 = distinct !DILexicalBlock(scope: !16, file: !1, line: 6, column: 3)
+  !20 = !DIExpression()
+  !21 = !DILocation(line: 2, column: 15, scope: !8)
+  !22 = !DILocation(line: 3, column: 17, scope: !8)
+  !23 = !DILocation(line: 3, column: 13, scope: !8)
+  !24 = !DILocation(line: 4, column: 6, scope: !17)
+  !25 = !DILocation(line: 4, column: 6, scope: !8)
+  !26 = !DILocation(line: 5, column: 11, scope: !16)
+  !27 = !DILocation(line: 5, column: 7, scope: !16)
+  !28 = !DILocation(line: 6, column: 11, scope: !19)
+  !29 = !DILocation(line: 6, column: 7, scope: !19)
+  !30 = !DILocation(line: 6, column: 20, scope: !31)
+  !31 = !DILexicalBlockFile(scope: !32, file: !1, discriminator: 1)
+  !32 = distinct !DILexicalBlock(scope: !19, file: !1, line: 6, column: 3)
+  !33 = !DILocation(line: 6, column: 3, scope: !31)
+  !34 = !DILocation(line: 9, column: 3, scope: !16)
+  !35 = !DILocation(line: 10, column: 2, scope: !16)
+  !36 = !DILocation(line: 7, column: 4, scope: !37)
+  !37 = distinct !DILexicalBlock(scope: !32, file: !1, line: 6, column: 30)
+  !38 = !DILocation(line: 6, column: 26, scope: !39)
+  !39 = !DILexicalBlockFile(scope: !32, file: !1, discriminator: 3)
+  !40 = !DILocation(line: 6, column: 3, scope: !39)
+  !41 = distinct !{!41, !42}
+  !42 = !DILocation(line: 6, column: 3, scope: !16)
+  !43 = !DILocation(line: 11, column: 2, scope: !8)
+  !44 = !DILocation(line: 12, column: 1, scope: !8)
+
+...
+---
+name:            func
+alignment:       2
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+tracksRegLiveness: false
+liveins:         
+  - { reg: '%r0' }
+calleeSavedRegisters: [ '%lr', '%d8', '%d9', '%d10', '%d11', '%d12', '%d13', 
+                        '%d14', '%d15', '%q4', '%q5', '%q6', '%q7', '%r4', 
+                        '%r5', '%r6', '%r7', '%r8', '%r9', '%r10', '%r11', 
+                        '%s16', '%s17', '%s18', '%s19', '%s20', '%s21', 
+                        '%s22', '%s23', '%s24', '%s25', '%s26', '%s27', 
+                        '%s28', '%s29', '%s30', '%s31', '%d8_d10', '%d9_d11', 
+                        '%d10_d12', '%d11_d13', '%d12_d14', '%d13_d15', 
+                        '%q4_q5', '%q5_q6', '%q6_q7', '%q4_q5_q6_q7', '%r4_r5', 
+                        '%r6_r7', '%r8_r9', '%r10_r11', '%d8_d9_d10', '%d9_d10_d11', 
+                        '%d10_d11_d12', '%d11_d12_d13', '%d12_d13_d14', 
+                        '%d13_d14_d15', '%d8_d10_d12', '%d9_d11_d13', '%d10_d12_d14', 
+                        '%d11_d13_d15', '%d8_d10_d12_d14', '%d9_d11_d13_d15', 
+                        '%d9_d10', '%d11_d12', '%d13_d14', '%d9_d10_d11_d12', 
+                        '%d11_d12_d13_d14' ]
+frameInfo:       
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       24
+  offsetAdjustment: 0
+  maxAlignment:    4
+  adjustsStack:    true
+  hasCalls:        true
+  maxCallFrameSize: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+stack:           
+  - { id: 0, type: spill-slot, offset: -4, size: 4, alignment: 4, callee-saved-register: '%lr' }
+  - { id: 1, type: spill-slot, offset: -8, size: 4, alignment: 4, callee-saved-register: '%r11' }
+  - { id: 2, type: spill-slot, offset: -12, size: 4, alignment: 4, callee-saved-register: '%r7' }
+  - { id: 3, type: spill-slot, offset: -16, size: 4, alignment: 4, callee-saved-register: '%r6' }
+  - { id: 4, type: spill-slot, offset: -20, size: 4, alignment: 4, callee-saved-register: '%r5' }
+  - { id: 5, type: spill-slot, offset: -24, size: 4, alignment: 4, callee-saved-register: '%r4' }
+body:             |
+  bb.0.entry:
+    successors: %bb.5.if.end, %bb.1.if.then
+    liveins: %r0, %r4, %r5, %r6, %r7, %r11, %lr
+  
+    %sp = frame-setup STMDB_UPD %sp, 14, _, killed %r4, killed %r5, killed %r6, killed %r7, killed %r11, killed %lr
+    frame-setup CFI_INSTRUCTION def_cfa_offset 24
+    frame-setup CFI_INSTRUCTION offset %lr, -4
+    frame-setup CFI_INSTRUCTION offset %r11, -8
+    frame-setup CFI_INSTRUCTION offset %r7, -12
+    frame-setup CFI_INSTRUCTION offset %r6, -16
+    frame-setup CFI_INSTRUCTION offset %r5, -20
+    frame-setup CFI_INSTRUCTION offset %r4, -24
+    DBG_VALUE debug-use %r0, debug-use _, !13, !20, debug-location !21
+    %r4 = MOVr killed %r0, 14, _, _
+    DBG_VALUE debug-use %r4, debug-use _, !13, !20, debug-location !21
+    %r0 = MOVi 10, 14, _, _, debug-location !22
+    %r1 = MOVi 11, 14, _, _, debug-location !22
+    BL @func2, csr_aapcs, implicit-def dead %lr, implicit %sp, implicit killed %r0, implicit killed %r1, implicit-def %sp, implicit-def %r0, debug-location !22
+    %r5 = MOVr killed %r0, 14, _, _, debug-location !22
+    DBG_VALUE debug-use %r5, debug-use _, !14, !20, debug-location !23
+    CMPri %r4, 0, 14, _, implicit-def %cpsr, debug-location !25
+    Bcc %bb.5.if.end, 0, killed %cpsr
+  
+  bb.1.if.then:
+    successors: %bb.3.for.cond
+    liveins: %r4, %r5
+  
+    %r0 = MOVi 12, 14, _, _, debug-location !26
+    %r1 = MOVi 13, 14, _, _, debug-location !26
+    BL @func2, csr_aapcs, implicit-def dead %lr, implicit %sp, implicit killed %r0, implicit killed %r1, implicit-def %sp, implicit-def %r0, debug-location !26
+    %r6 = MOVr killed %r0, 14, _, _, debug-location !26
+    DBG_VALUE debug-use %r6, debug-use _, !15, !20, debug-location !27
+    %r7 = MOVi 1, 14, _, _
+    DBG_VALUE 1, 0, !18, !20, debug-location !28
+    B %bb.3.for.cond
+  
+  bb.2.for.body:
+    successors: %bb.3.for.cond
+    liveins: %r4, %r5, %r6, %r7
+  
+    %r1 = ADDrr %r5, %r7, 14, _, _, debug-location !36
+    %r0 = MOVr %r7, 14, _, _, debug-location !36
+    BL @func2, csr_aapcs, implicit-def dead %lr, implicit %sp, implicit killed %r0, implicit killed %r1, implicit-def %sp, implicit-def dead %r0, debug-location !36
+    %r7 = ADDri killed %r7, 1, 14, _, _, debug-location !38
+    DBG_VALUE debug-use %r7, debug-use _, !18, !20, debug-location !28
+  
+  bb.3.for.cond:
+    successors: %bb.2.for.body, %bb.4.for.cond.cleanup
+    liveins: %r4, %r5, %r6, %r7
+  
+    DBG_VALUE debug-use %r7, debug-use _, !18, !20, debug-location !28
+    CMPrr %r7, %r4, 14, _, implicit-def %cpsr, debug-location !33
+    Bcc %bb.2.for.body, 11, killed %cpsr, debug-location !33
+  
+  bb.4.for.cond.cleanup:
+    successors: %bb.5.if.end
+    liveins: %r4, %r5, %r6
+  
+    %r0 = MOVr %r5, 14, _, _, debug-location !34
+    %r1 = MOVr killed %r6, 14, _, _, debug-location !34
+    BL @func2, csr_aapcs, implicit-def dead %lr, implicit %sp, implicit killed %r0, implicit killed %r1, implicit-def %sp, implicit-def dead %r0, debug-location !34
+  
+  bb.5.if.end:
+    liveins: %r4, %r5
+  
+    %r0 = MOVr killed %r5, 14, _, _, debug-location !43
+    %r1 = MOVr killed %r4, 14, _, _, debug-location !43
+    %sp = LDMIA_UPD %sp, 14, _, def %r4, def %r5, def %r6, def %r7, def %r11, def %lr, debug-location !43
+    TAILJMPd @func2, implicit %sp, implicit %sp, implicit killed %r0, implicit killed %r1, debug-location !43
+
+...