Make MachineBasicBlock::updateTerminator to update DebugLoc as well

[twoh]

Summary

Currently MachineBasicBlock::updateTerminator simply drops DebugLoc for newly created branch instructions, which may cause incorrect stepping and/or imprecise sample profile data. Below is an example:

 1 extern int bar(int x);
 2
 3 int foo(int *begin, int *end) {
 4   int *i;
 5   int ret = 0;
 6   for (
 7       i = begin ;
 8       i != end ;
 9       i++)
10   {
11       ret += bar(*i);
12   }
13   return ret;
14 }

Below is a bitcode of 'foo' at the end of LLVM-IR level optimizations with -O3:

define i32 @foo(i32* readonly %begin, i32* readnone %end) !dbg !4 {
entry:
  %cmp6 = icmp eq i32* %begin, %end, !dbg !9
  br i1 %cmp6, label %for.end, label %for.body.preheader, !dbg !12

for.body.preheader:                               ; preds = %entry
  br label %for.body, !dbg !13

for.body:                                         ; preds = %for.body.preheader, %for.body
  %ret.08 = phi i32 [ %add, %for.body ], [ 0, %for.body.preheader ]
  %i.07 = phi i32* [ %incdec.ptr, %for.body ], [ %begin, %for.body.preheader ]
  %0 = load i32, i32* %i.07, align 4, !dbg !13, !tbaa !15
  %call = tail call i32 @bar(i32 %0), !dbg !19
  %add = add nsw i32 %call, %ret.08, !dbg !20
  %incdec.ptr = getelementptr inbounds i32, i32* %i.07, i64 1, !dbg !21
  %cmp = icmp eq i32* %incdec.ptr, %end, !dbg !9
  br i1 %cmp, label %for.end.loopexit, label %for.body, !dbg !12, !llvm.loop !22

for.end.loopexit:                                 ; preds = %for.body
  br label %for.end, !dbg !24

for.end:                                          ; preds = %for.end.loopexit, %entry
  %ret.0.lcssa = phi i32 [ 0, %entry ], [ %add, %for.end.loopexit ]
  ret i32 %ret.0.lcssa, !dbg !24
}

where

!12 = !DILocation(line: 6, column: 3, scope: !11)

. As you can see, the terminator of 'entry' block, which is a loop control branch, has a DebugLoc of line 6, column 3. Howerver, after the execution of 'MachineBlock::updateTerminator' function, which is triggered by MachineSinking pass, the DebugLoc info is dropped as below (see there's no debug-location for JNE_1):

bb.0.entry:
  successors: %bb.4(0x30000000), %bb.1.for.body.preheader(0x50000000)
  liveins: %rdi, %rsi

  %6 = COPY %rsi
  %5 = COPY %rdi
  %8 = SUB64rr %5, %6, implicit-def %eflags, debug-location !9
  JNE_1 %bb.1.for.body.preheader, implicit %eflags

This patch addresses this issue and make newly created branch instructions to keep debug-location info.

[twoh]

• Merge branch 'master' into mbb_metadata

[twoh]

Implemented a separate 'findBranchDebugLoc'. This functions does essentially the same job as 'getBranchDebugLoc' function in BranchFolding.cpp. Unlike getBranchDebugLoc, the new findBranchDebugLoc finds all branch instructions inside the function and returns the merged DebugLoc of them. I think it is more conservative, Though I can't imagine the case of two branch instructions in the same machine basic block have different DebugLoc.

[twoh]

Friendly ping. Thanks!

[qcolombet]

LGTM

[dblaikie]

llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp

1158 ↗

(On Diff #88223)

probably leave off the {} if they aren't needed?

[twoh]

@dblaikie Make sense. I addressed it. Thanks!

lib/CodeGen/MachineBasicBlock.cpp

 #include "llvm/CodeGen/MachineDominators.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineLoopInfo.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/SlotIndexes.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/DataLayout.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/ModuleSlotTracker.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/Support/DataTypes.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetInstrInfo.h"
 #include "llvm/Target/TargetMachine.h"

 void MachineBasicBlock::updateTerminator() {
   const TargetInstrInfo *TII = getParent()->getSubtarget().getInstrInfo();
   // A block with no successors has no concerns with fall-through edges.
   if (this->succ_empty())
     return;
 
   MachineBasicBlock *TBB = nullptr, *FBB = nullptr;
   SmallVector<MachineOperand, 4> Cond;
-  DebugLoc DL;  // FIXME: this is nowhere
+  DebugLoc DL;
+  auto TI = getFirstTerminator();
+  if (TI != end()) {
+    DL = TI->getDebugLoc();
+    for (++TI ; TI != end() ; ++TI)
+      DL = DILocation::getMergedLocation(DL, TI->getDebugLoc());
+  }
   bool B = TII->analyzeBranch(*this, TBB, FBB, Cond);
   (void) B;
   assert(!B && "UpdateTerminators requires analyzable predecessors!");
   if (Cond.empty()) {
     if (TBB) {
       // The block has an unconditional branch. If its successor is now its
       // layout successor, delete the branch.
       if (isLayoutSuccessor(TBB))
void MachineBasicBlock::updateTerminator() {
 
   if (!FallthroughBB) {
     if (canFallThrough()) {
       // We fallthrough to the same basic block as the conditional jump targets.
       // Remove the conditional jump, leaving unconditional fallthrough.
       // FIXME: This does not seem like a reasonable pattern to support, but it
       // has been seen in the wild coming out of degenerate ARM test cases.
       TII->removeBranch(*this);
-  
       // Finally update the unconditional successor to be reached via a branch if
       // it would not be reached by fallthrough.
       if (!isLayoutSuccessor(TBB))
         TII->insertBranch(*this, TBB, nullptr, Cond, DL);
       return;
     }
 
     // We enter here iff exactly one successor is TBB which cannot fallthrough

test/CodeGen/X86/update-terminator-debugloc.ll

+; RUN: llc -stop-after=machine-sink -march=x86-64 < %s | FileCheck %s
+;
+; test code:
+;  1 extern int bar(int x);
+;  2
+;  3 int foo(int *begin, int *end) {
+;  4   int *i;
+;  5   int ret = 0;
+;  6   for (
+;  7       i = begin ;
+;  8       i != end ;
+;  9       i++)
+; 10   {
+; 11       ret += bar(*i);
+; 12   }
+; 13   return ret;
+; 14 }
+; 
+; With the test code, LLVM-IR below shows that loop-control branches have a 
+; debug location of line 6 (branches in entry and for.body block). Make sure that
+; these debug locations are propaged correctly to lowered instructions.
+;
+; CHECK: [[DLOC:![0-9]+]] = !DILocation(line: 6
+; CHECK-DAG: [[VREG1:%[^ ]+]] = COPY %rsi
+; CHECK-DAG: [[VREG2:%[^ ]+]] = COPY %rdi
+; CHECK: SUB64rr [[VREG2]], [[VREG1]]
+; CHECK-NEXT: JNE_1 {{.*}}, debug-location [[DLOC]]{{$}}
+; CHECK: [[VREG3:%[^ ]+]] = PHI [[VREG2]]
+; CHECK: [[VREG4:%[^ ]+]] = ADD64ri8 [[VREG3]], 4
+; CHECK: SUB64rr [[VREG1]], [[VREG4]]
+; CHECK-NEXT: JNE_1 {{.*}}, debug-location [[DLOC]]{{$}}
+; CHECK-NEXT: JMP_1 {{.*}}, debug-location [[DLOC]]{{$}}
+
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @foo(i32* readonly %begin, i32* readnone %end) !dbg !4 {
+entry:
+  %cmp6 = icmp eq i32* %begin, %end, !dbg !9
+  br i1 %cmp6, label %for.end, label %for.body.preheader, !dbg !12
+
+for.body.preheader:                               ; preds = %entry
+  br label %for.body, !dbg !13
+
+for.body:                                         ; preds = %for.body.preheader, %for.body
+  %ret.08 = phi i32 [ %add, %for.body ], [ 0, %for.body.preheader ]
+  %i.07 = phi i32* [ %incdec.ptr, %for.body ], [ %begin, %for.body.preheader ]
+  %0 = load i32, i32* %i.07, align 4, !dbg !13, !tbaa !15
+  %call = tail call i32 @bar(i32 %0), !dbg !19
+  %add = add nsw i32 %call, %ret.08, !dbg !20
+  %incdec.ptr = getelementptr inbounds i32, i32* %i.07, i64 1, !dbg !21
+  %cmp = icmp eq i32* %incdec.ptr, %end, !dbg !9
+  br i1 %cmp, label %for.end.loopexit, label %for.body, !dbg !12, !llvm.loop !22
+
+for.end.loopexit:                                 ; preds = %for.body
+  br label %for.end, !dbg !24
+
+for.end:                                          ; preds = %for.end.loopexit, %entry
+  %ret.0.lcssa = phi i32 [ 0, %entry ], [ %add, %for.end.loopexit ]
+  ret i32 %ret.0.lcssa, !dbg !24
+}
+
+declare i32 @bar(i32)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+!1 = !DIFile(filename: "foo.c", directory: "b/")
+!2 = !{i32 2, !"Dwarf Version", i32 4}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 3, type: !5, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: true, unit: !0)
+!5 = !DISubroutineType(types: !6)
+!6 = !{!7, !8, !8}
+!7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!8 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !7, size: 64)
+!9 = !DILocation(line: 8, column: 9, scope: !10)
+!10 = distinct !DILexicalBlock(scope: !11, file: !1, line: 6, column: 3)
+!11 = distinct !DILexicalBlock(scope: !4, file: !1, line: 6, column: 3)
+!12 = !DILocation(line: 6, column: 3, scope: !11)
+!13 = !DILocation(line: 11, column: 18, scope: !14)
+!14 = distinct !DILexicalBlock(scope: !10, file: !1, line: 10, column: 3)
+!15 = !{!16, !16, i64 0}
+!16 = !{!"int", !17, i64 0}
+!17 = !{!"omnipotent char", !18, i64 0}
+!18 = !{!"Simple C/C++ TBAA"}
+!19 = !DILocation(line: 11, column: 14, scope: !14)
+!20 = !DILocation(line: 11, column: 11, scope: !14)
+!21 = !DILocation(line: 9, column: 8, scope: !10)
+!22 = distinct !{!22, !12, !23}
+!23 = !DILocation(line: 12, column: 3, scope: !11)
+!24 = !DILocation(line: 13, column: 3, scope: !4)