[llvm-commits] [llvm] r60343 - /llvm/trunk/lib/Transforms/Scalar/InstructionCombining.cpp

[Chris Lattner]

On Dec 1, 2008, at 12:23 AM, Bill Wendling wrote:

> Author: void
> Date: Mon Dec  1 02:23:25 2008
> New Revision: 60343
>
> URL: http://llvm.org/viewvc/llvm-project?rev=60343&view=rev
> Log:
> Reduce copy-and-paste code by splitting out the code into its own  
> function.

Thanks!  Next step:

> +Instruction *InstCombiner::FoldOrWithConstants(BinaryOperator &I,
> +                                               Value *A, Value *B,  
> Value *C) {
> +  Value *Op1 = I.getOperand(1);
> +
> +  if (ConstantInt *CI = dyn_cast<ConstantInt>(C)) {
> +    if (CI->getValue() == 1) {

Now that this is a function, there is no need to nest this.  Just use  
early outs to keep it linear and unnested.

>
> +      Value *V1 = 0, *C2 = 0;
> +      if (match(Op1, m_And(m_Value(V1), m_Value(C2)))) {
> +        ConstantInt *CI2 = dyn_cast<ConstantInt>(C2);
> +
> +        if (!CI2) {
> +          std::swap(V1, C2);
> +          CI2 = dyn_cast<ConstantInt>(C2);
> +        }

No need to test for this.  and(cst, V) will be canonicalized to and(V,  
cst).  All commutative operations commute constants to their RHS.

>
> +
> +        if (CI2) {
> +          APInt NegTwo = -APInt(CI2->getValue().getBitWidth(), 2,  
> true);

You're still testing magic constants.  Please generalize this.

>
> +          if (CI2->getValue().eq(NegTwo)) {

I think you can use == instead of .eq.


>
> +            if (V1 == B) {
> +              Instruction *NewOp =
> +                InsertNewInstBefore(BinaryOperator::CreateAnd(A,  
> CI), I);
> +              return BinaryOperator::CreateOr(NewOp, B);
> +            }
> +            if (V1 == A) {
> +              Instruction *NewOp =
> +                InsertNewInstBefore(BinaryOperator::CreateAnd(B,  
> CI), I);
> +              return BinaryOperator::CreateOr(NewOp, A);
> +            }

Please merge these two if's.

-Chris