[PATCH] Optimize icmp involving addition better

[David Majnemer]

Thanks for taking a look.

> Hi David,
> 
>> --- lib/Transforms/InstCombine/InstCombineCompares.cpp	(revision 179146)
>> +++ lib/Transforms/InstCombine/InstCombineCompares.cpp	(working copy)
>> @@ -2487,6 +2487,59 @@
>>       return new ICmpInst(Pred, Y, Z);
>>     }
>> 
>> +    // icmp slt (X + -1), Y -> icmp sle X, Y
>> +    if (A && NoOp0WrapProblem && match(B, m_AllOnes()) &&
>> +        Pred == CmpInst::ICMP_SLT)
> 
> it would be cheaper to first check the predicate Pred before doing the match,
> rather than after, so that if the predicate is wrong then the match code
> isn't executed.

Changed.

> 
>> +    // if C1 has greater magnitude than C2:
>> +    //  icmp (X + C1), (Y + C2) -> icmp (X + C3), Y
> 
> You didn't define C3 (C3 = C1 - C2 here).  You also forgot to mention that
> they have to have the same sign.

Changed.

> 
>> +    //
>> +    // if C2 has greater magnitude than C1:
>> +    // icmp (X + C1), (Y + C2) -> icmp X, (Y + C3)
> 
> Likewise.


Changed.

> 
>> +    if (A && C && NoOp0WrapProblem && NoOp1WrapProblem &&
>> +        (BO0->hasOneUse() || BO1->hasOneUse()) &&
>> +        ICmpInst::getSignedPredicate(Pred) == Pred)
> 
> You can use isSigned(Pred) rather than this last test.

Your expression is different than mine in that it excludes the possibilities of pred being ICMP_EQ or ICMP_NE.
My goal was to exclude the optimization from deal with unsigned comparisons, I've changed it to !I.isUnsigned()

> 
>> +      if (ConstantInt *C1 = dyn_cast<ConstantInt>(B))
>> +        if (ConstantInt *C2 = dyn_cast<ConstantInt>(D)) {
>> +          const APInt &AP1 = C1->getValue();
>> +          const APInt &AP2 = C2->getValue();
>> +          if (AP1.isNegative() == AP2.isNegative()) {
>> +            APInt AP1Abs = C1->getValue().abs();
>> +            APInt AP2Abs = C2->getValue().abs();
>> +            APInt AP3;
>> +            if (AP1Abs.uge(AP2Abs))
>> +              AP3 = AP1Abs - AP2Abs;
> 
> Can't this be: AP3 = AP1 - AP2

Changed.

> 
>> +            else
>> +              AP3 = AP2Abs - AP1Abs;
> 
> And here: AP3 = AP2 - AP1

Changed.

> 
>> +            if (AP1.isNegative())
>> +              AP3 = -AP3;
> 
> Then this can go away.

Changed.

> 
>> +
>> +            ConstantInt *C3 = Builder->getInt(AP3);
>> +            Value *Modded = AP1Abs.uge(AP2Abs) ? A : C;
>> +            Value *NewAdd = Builder->CreateNSWAdd(Modded, C3);
>> +            Value *NewLHS = Modded == A ? NewAdd : A;
>> +            Value *NewRHS = Modded == C ? NewAdd : C;
>> +            return new ICmpInst(Pred, NewLHS, NewRHS);
> 
> Instead of this, it might be simpler to do:
> 
>  if (AP1Abs.uge(AP2Abs)) {
>    AP3 = AP1 - AP2;
>    ConstantInt *C3 = Builder->getInt(AP3); // Could just use AP1-AP2 directly
>    Value *NewAdd = Builder->CreateNSWAdd(A, C3);
>    return new ICmpInst(Pred, NewAdd, C);
>  } else {
> ... likewise ...
>  }
> 
> Ciao, Duncan.
>> +          }
>> +        }
>> +
>> +
>>     // Analyze the case when either Op0 or Op1 is a sub instruction.
>>     // Op0 = A - B (or A and B are null); Op1 = C - D (or C and D are null).
>>     A = 0; B = 0; C = 0; D = 0;
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
A non-text attachment was scrubbed...
Name: icmp_cstv2.diff
Type: application/octet-stream
Size: 2504 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130411/be9b0543/attachment.obj>