Page MenuHomePhabricator

[JumpThreading] Retain debug info when replacing branch instructions
ClosedPublic

Authored by StephenTozer on Mar 5 2019, 6:29 AM.

Details

Summary

Fixes bug 37966: https://bugs.llvm.org/show_bug.cgi?id=37966

The Jump Threading pass will replace certain conditional branch
instructions with unconditional branches when it can prove that only one
branch can occur. Prior to this patch, it would not carry the debug
info from the old instruction to the new one.

This patch fixes the bug described by copying the debug info from the
conditional branch instruction to the new unconditional branch
instruction, and adds a regression test for the Jump Threading pass that
covers this case.

Diff Detail

Repository
rL LLVM

Event Timeline

StephenTozer created this revision.Mar 5 2019, 6:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2019, 6:29 AM
gbedwell added a subscriber: vsk.Mar 5 2019, 6:47 AM
brzycki accepted this revision.Mar 6 2019, 7:41 AM

LGTM. It's a simple and clean change.

This revision is now accepted and ready to land.Mar 6 2019, 7:41 AM

The code change touches two places, but the test only seems to test one branch instruction - is one of the two code changes untested?

The code change touches two places, but the test only seems to test one branch instruction - is one of the two code changes untested?

Yes; the second case seemed like it would need an additional, more complicated IR case to test, but the logic for performing the conditional-to-unconditional branch conversion is identical in both cases (could be extracted to a utility function perhaps?) and the semantic meaning is the same as well: "The condition at this state is guaranteed to be true/false, so remove the condition check".

The code change touches two places, but the test only seems to test one branch instruction - is one of the two code changes untested?

Yes; the second case seemed like it would need an additional, more complicated IR case to test, but the logic for performing the conditional-to-unconditional branch conversion is identical in both cases (could be extracted to a utility function perhaps?) and the semantic meaning is the same as well: "The condition at this state is guaranteed to be true/false, so remove the condition check".

Hi @StephenTozer,
I agree with @dblaikie it would be good to have a test-case for both locations. I do understand the logic is the same in both but the general rule is to add test-cases for all code path coverage in changed code. I added an assert() to your patch in the following location:

@@ -1245,7 +1247,9 @@ bool JumpThreadingPass::ProcessImpliedCondition(BasicBlock *BB) {
       BasicBlock *KeepSucc = BI->getSuccessor(*Implication ? 0 : 1);
       BasicBlock *RemoveSucc = BI->getSuccessor(*Implication ? 1 : 0);
       RemoveSucc->removePredecessor(BB);
-      BranchInst::Create(KeepSucc, BI);
+      BranchInst *UncondBI = BranchInst::Create(KeepSucc, BI);
+      UncondBI->setDebugLoc(BI->getDebugLoc());
+      assert(0 && "BMR");
       BI->eraseFromParent();
       DTU->applyUpdatesPermissive({{DominatorTree::Delete, BB, RemoveSucc}});
       return true;

and ran make check on the code. I found an implied condition test-case that is pretty short exercising this code path. Do you know how to add debug info to the IR to check the before and after states? Here's the test:

; RUN: opt -jump-threading -S < %s | FileCheck %s

declare void @side_effect(i32)

define void @test0(i32 %i, i32 %len) {
; CHECK-LABEL: @test0(
 entry:
  call void @side_effect(i32 0)
  %i.inc = add nuw i32 %i, 1
  %c0 = icmp ult i32 %i.inc, %len
  br i1 %c0, label %left, label %right

 left:
; CHECK: entry:
; CHECK: br i1 %c0, label %left0, label %right

; CHECK: left0:
; CHECK: call void @side_effect
; CHECK-NOT: br i1 %c1
; CHECK: call void @side_effect
  call void @side_effect(i32 0)
  %c1 = icmp ult i32 %i, %len
  br i1 %c1, label %left0, label %right

 left0:
  call void @side_effect(i32 0)
  ret void

 right:
  %t = phi i32 [ 1, %left ], [ 2, %entry ]
  call void @side_effect(i32 %t)
  ret void
}

Added the test case for the second change, and reduced both it and the original test case as much as possible.

brzycki accepted this revision.Mar 8 2019, 11:06 AM

Thank you @StephenTozer , LGTM.

vsk accepted this revision.Mar 8 2019, 11:44 AM

Thanks, lgtm as well.

This revision was automatically updated to reflect the committed changes.