This is an archive of the discontinued LLVM Phabricator instance.

Make MachineBasicBlock::updateTerminator to update DebugLoc as well
ClosedPublic

Authored by twoh on Feb 6 2017, 11:18 AM.

Details

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.

Event Timeline

twoh created this revision.Feb 6 2017, 11:18 AM
twoh updated this revision to Diff 87466.Feb 7 2017, 9:10 AM
  • Merge branch 'master' into mbb_metadata
twoh updated this revision to Diff 87470.Feb 7 2017, 9:21 AM

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 added a comment.Feb 13 2017, 9:18 AM

Friendly ping. Thanks!

qcolombet accepted this revision.Feb 13 2017, 9:27 AM
qcolombet added a subscriber: qcolombet.

LGTM

This revision is now accepted and ready to land.Feb 13 2017, 9:27 AM
This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.
llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp
1158 ↗(On Diff #88223)

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

twoh added a comment.Feb 13 2017, 1:24 PM

@dblaikie Make sense. I addressed it. Thanks!