[PATCH] D58963: [JumpThreading] Retain debug info when replacing branch instructions

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

In D58963#1421264 <https://reviews.llvm.org/D58963#1421264>, @StephenTozer wrote:

> In D58963#1419867 <https://reviews.llvm.org/D58963#1419867>, @dblaikie wrote:
>
> > 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
  }


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58963/new/

https://reviews.llvm.org/D58963