Page MenuHomePhabricator

[SimplifyCFG] In sinkLastInstruction correctly set the debug location of the "common" instruction
ClosedPublic

Authored by rob.lougher on Dec 8 2016, 1:40 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

rob.lougher updated this revision to Diff 80816.Dec 8 2016, 1:40 PM
rob.lougher retitled this revision from to [SimplifyCFG] In sinkLastInstruction correctly set the debug location of the "common" instruction.
rob.lougher updated this object.
rob.lougher updated this object.Dec 8 2016, 1:43 PM
dblaikie accepted this revision.Dec 8 2016, 1:54 PM
dblaikie edited edge metadata.

(just out of curiosity/confirmation - I'd like to know that test fails without the check, I'm not 100% confident on my FileCheck emulation in my head to verify that it'll catch the bug)

This revision is now accepted and ready to land.Dec 8 2016, 1:54 PM
jmolloy accepted this revision.Dec 8 2016, 1:57 PM
jmolloy edited edge metadata.

LGTM too, FWIW!

Thanks for the review!

(just out of curiosity/confirmation - I'd like to know that test fails without the check, I'm not 100% confident on my FileCheck emulation in my head to verify that it'll catch the bug)

$ opt -simplifycfg simplifycfg_sink_last_inst.ll -S -o - | FileCheck simplifycfg_sink_last_inst.ll 
<stdin>:21:39: error: CHECK-NOT: string occurred!
 %sub2 = sub nsw i32 %b, %call1.sink, !dbg !11
                                      ^
simplifycfg_sink_last_inst.ll:28:14: note: CHECK-NOT: pattern specified here
; CHECK-NOT: !dbg
             ^

LGTM too, FWIW!

Thanks for the review! I can now commit as the dependent change is done.

This revision was automatically updated to reflect the committed changes.