[PATCH] Optimize icmp involving addition better

[Duncan Sands]

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.

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

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

Likewise.

> +    if (A && C && NoOp0WrapProblem && NoOp1WrapProblem &&
> +        (BO0->hasOneUse() || BO1->hasOneUse()) &&
> +        ICmpInst::getSignedPredicate(Pred) == Pred)

You can use isSigned(Pred) rather than this last test.

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

> +            else
> +              AP3 = AP2Abs - AP1Abs;

And here: AP3 = AP2 - AP1

> +            if (AP1.isNegative())
> +              AP3 = -AP3;

Then this can go away.

> +
> +            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;