[PATCH] D26256: [InstCombine] Don't set debug location when folding through a phi node

[Robert Lougher via llvm-commits]

rob.lougher created this revision.
rob.lougher added reviewers: dblaikie, aprantl, danielcdh, wolfgangp, probinson.
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 InstCombine, the two sub instructions feeding into the phi node will be combined into a single sub with an incoming phi node:

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

However, the combined sub has been given the debug location of the first instruction:

  !11 = !DILocation(line: 10, column: 7, scope: !5)

This is a problem for sample-based PGO as hits on the sub instruction will be counted towards the if-part of the if-then-else even when the else-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.

This patch fixes the issue by removing the line that sets the common instruction debug location (as the location is now ambiguous).

In addition to binary operations, instcombine will also fold various others such as casts, loads and getelementptr through a phi node (in total, InstCombinePHI.cpp contains 7 calls to set the debug location on the combined instruction).  The patch removes all of these.  A test is included which contains 7 tests that check each of these cases.

OK to commit? Although the situation is the same for each case, I can split the patch and commit each change separately if preferred.

Thanks,
Rob.


https://reviews.llvm.org/D26256

Files:
  lib/Transforms/InstCombine/InstCombinePHI.cpp
  test/DebugInfo/Generic/instcombine-phi.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D26256.76764.patch
Type: text/x-patch
Size: 15905 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161102/4c3dd000/attachment.bin>