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

[Bill Wendling]

On Dec 1, 2008, at 9:00 PM, Chris Lattner wrote:

>
> 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.
>
Okay.

>>
>> +      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.
>
Ah! good to know.

>>
>> +
>> +        if (CI2) {
>> +          APInt NegTwo = -APInt(CI2->getValue().getBitWidth(), 2,
>> true);
>
> You're still testing magic constants.  Please generalize this.
>
I did in a following patch.

>> +            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.
>
Okay.

-bw