[PATCH] D27590: [SimplifyCFG] In sinkLastInstruction correctly set the debug location of the "common" instruction

[Robert Lougher via Phabricator via llvm-commits]

rob.lougher created this revision.
rob.lougher added reviewers: dblaikie, aprantl, danielcdh, probinson, wolfgangp.
rob.lougher added subscribers: llvm-commits, andreadb, gbedwell.

Consider the following simple if-then-else:

  define i32 @test(i32 %a, i32 %b) !dbg !6 {
  entry:
    %tobool = icmp ne i32 %a, 0, !dbg !8
    br i1 %tobool, label %if.then, label %if.else, !dbg !8
  
  if.then:                                          ; preds = %entry
    %call = call i32 @foo(), !dbg !9
    %sub = sub nsw i32 %b, %call, !dbg !10
    br label %if.end, !dbg !11
  
  if.else:                                          ; preds = %entry
    %call1 = call i32 @bar(), !dbg !12
    %sub2 = sub nsw i32 %b, %call1, !dbg !13
    br label %if.end
  
  if.end:                                           ; preds = %if.else, %if.then
    %b.addr.0 = phi i32 [ %sub, %if.then ], [ %sub2, %if.else ]
    ret i32 %b.addr.0, !dbg !14
  }

With the source location of the sub instructions described by:

  !10 = !DILocation(line: 10, column: 7, scope: !6)
  !13 = !DILocation(line: 12, column: 7, scope: !6)

If this is passed to simplify CFG, the two sub instructions will be sunk into the successor block (sinkLastInstruction):

  if.end:                                           ; preds = %if.else, %if.then
    %call1.sink = phi i32 [ %call1, %if.else ], [ %call, %if.then ]
    %sub2 = sub nsw i32 %b, %call1.sink, !dbg !11
    ret i32 %sub2, !dbg !12

However, the "common" sub has the debug location of the second instruction:

    !11 = !DILocation(line: 12, column: 7, scope: !5)
  ``
  This is a problem for sample-based PGO as hits on the sub instruction will be counted towards the else-part of the if-then-else even when the if-part was executed (which may lead to incorrect decisions to re-order blocks). It will also affect the optimized debugging experience leading to odd stepping in the debugger.
  
  The problem occurs because sinkLastInstruction arbitrarily selects one of the instructions and moves it to the successor.  When it does this it does not update the debug location so it keeps its original location.
  
  This patch fixes the issue by setting the debug location to the result of merging the debug locations of the commoned instructions.  This uses the new API discussed in D26256 (which is still under discussion).
  
  Thanks,
  Rob.


https://reviews.llvm.org/D27590

Files:
  lib/Transforms/Utils/SimplifyCFG.cpp
  test/DebugInfo/Generic/simplifycfg_sink_last_inst.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D27590.80816.patch
Type: text/x-patch
Size: 4035 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161208/3b5f02c2/attachment.bin>