[llvm-commits] [llvm] r112270 - in /llvm/trunk: lib/Analysis/LazyValueInfo.cpp lib/Transforms/Scalar/JumpThreading.cpp test/Transforms/JumpThreading/basic.ll test/Transforms/JumpThreading/lvi-load.ll

[Chris Lattner]

On Aug 27, 2010, at 10:12 AM, Owen Anderson wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=112270&view=rev
> Log:
> Use LVI to eliminate conditional branches where we've tested a related condition previously.  Update tests for this change.
> This fixes PR5652.

Ok.

> +      // For predecessor edge, determine if the comparison is true or false
> +      // on that edge.  If they're all true or all false, we can simplify the
> +      // branch.
> +      // FIXME: We could handle mixed true/false by duplicating code.
> +      unsigned Trues = 0, Falses = 0, predcount = 0;
> +      for (pred_iterator PI = pred_begin(BB), PE = pred_end(BB);PI != PE; ++PI){
> +        ++predcount;
> +        LazyValueInfo::Tristate Ret =
> +          LVI->getPredicateOnEdge(CondCmp->getPredicate(), 
> +                                  CondCmp->getOperand(0), CondConst, *PI, BB);
> +        if (Ret == LazyValueInfo::True)
> +          ++Trues;
> +        else if (Ret == LazyValueInfo::False)
> +          ++Falses;
> +      }

This loop is serious overkill.  You evaluate getPredicateOnEdge for every predecessor, even when it is clear that nothing will be done.

It would be better to factor this out into a helper function, then call it once for the first pred.  If the first pred is true/false, walk through the rest of the preds while they are true/false.  If ever there is a mismatch, bail out.

> +      // If we can determine the branch direction statically, converted
> +      // the conditional branch to an unconditional one.
> +      if (Trues && Trues == predcount) {
> +        RemovePredecessorAndSimplify(CondBr->getSuccessor(1), BB, TD);
> +        BranchInst::Create(CondBr->getSuccessor(0), CondBr);
> +        CondBr->eraseFromParent();
> +        return true;
> +      } else if (Falses && Falses == predcount) {

Minor nit, you don't need an 'else' after a return.

> +++ llvm/trunk/test/Transforms/JumpThreading/basic.ll Fri Aug 27 12:12:29 2010
> @@ -147,11 +147,17 @@

I see that this changes some tests (so you can claim that it is tested), but it also seems that you should be adding a specific test (e.g. to basic.ll) with an example distilled from PR5652.

-Chris

> ; CHECK: @test6
> 	%tmp455 = icmp eq i32 %A, 42
> 	br i1 %tmp455, label %BB1, label %BB2
> -        
> -BB2:
> +
> +; CHECK: call i32 @f2()
> +; CHECK-NEXT: ret i32 3
> +
> ; CHECK: call i32 @f1()
> -; CHECK-NEXT: call void @f3()
> -; CHECK-NEXT: ret i32 4
> +; CHECK-NOT: br
> +; CHECK: call void @f3()
> +; CHECK-NOT: br
> +; CHECK: ret i32 4
> +    
> +BB2:
> 	call i32 @f1()
> 	br label %BB1
> 
> 
> Modified: llvm/trunk/test/Transforms/JumpThreading/lvi-load.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/JumpThreading/lvi-load.ll?rev=112270&r1=112269&r2=112270&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/JumpThreading/lvi-load.ll (original)
> +++ llvm/trunk/test/Transforms/JumpThreading/lvi-load.ll Fri Aug 27 12:12:29 2010
> @@ -1,4 +1,4 @@
> -; RUN: opt -S -jump-threading -enable-jump-threading-lvi < %s | FileCheck %s
> +; RUN: opt -S -jump-threading -enable-jump-threading-lvi -dce < %s | FileCheck %s
> target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
> target triple = "x86_64-apple-darwin10.4"
> 
> @@ -25,6 +25,7 @@
>   %toBoolnot.i.i = icmp ult i8 %1, 21             ; <i1> [#uses=1]
>   br i1 %toBoolnot.i.i, label %bb6.i.i, label %_ZN4llvm8dyn_castINS_11InstructionEPNS_5ValueEEENS_10cast_rettyIT_T0_E8ret_typeERKS6_.exit
> 
> +; CHECK-NOT: assert
> bb6.i.i:                                          ; preds = %bb.i
>   tail call void @__assert_rtn(i8* getelementptr inbounds ([5 x i8]* @_ZZN4llvm4castINS_11InstructionEPNS_5ValueEEENS_10cast_rettyIT_T0_E8ret_typeERKS6_E8__func__, i64 0, i64 0), i8* getelementptr inbounds ([31 x i8]* @.str, i64 0, i64 0), i32 202, i8* getelementptr inbounds ([59 x i8]* @.str1, i64 0, i64 0)) noreturn
>   unreachable
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits