[llvm] r177856 - InstCombine: simplify comparisons to zero of (shl %x, Cst) or (mul %x, Cst)

[Duncan Sands]

Hi Arnaud,

 > --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp Mon Mar 25 04:48:49 2013
> @@ -139,6 +139,42 @@ static bool isSignBitCheck(ICmpInst::Pre
>     }
>   }
>
> +/// Returns true if the exploded icmp can be expressed as a comparison to zero
> +/// and update the predicate accordingly. The signedness of the comparison is

This comment is incomplete.

> +static bool isSignTest(ICmpInst::Predicate &pred, const ConstantInt *RHS) {
> +  if (!ICmpInst::isSigned(pred))
> +    return false;
> +
> +  if (RHS->isZero())
> +    return true;
> +
> +  if (RHS->isOne())
> +    switch (pred) {
> +    case ICmpInst::ICMP_SGE:

Is this case needed?  I thought instcombine normalized SGE to SGT whenever
possible.  Likewise for SLE below.

> +      pred = ICmpInst::ICMP_SGT;
> +      return true;
> +    case ICmpInst::ICMP_SLT:
> +      pred = ICmpInst::ICMP_SLE;
> +      return true;
> +    default:
> +      return false;
> +    }
> +
> +  if (RHS->isAllOnesValue())
> +    switch (pred) {
> +    case ICmpInst::ICMP_SLE:
> +      pred = ICmpInst::ICMP_SLT;
> +      return true;
> +    case ICmpInst::ICMP_SGT:
> +      pred = ICmpInst::ICMP_SGE;
> +      return true;
> +    default:
> +      return false;
> +    }
> +
> +  return false;
> +}
> +
>   // isHighOnes - Return true if the constant is of the form 1+0+.
>   // This is the same as lowones(~X).
>   static bool isHighOnes(const ConstantInt *CI) {
> @@ -1282,6 +1318,25 @@ Instruction *InstCombiner::visitICmpInst
>       break;
>     }
>
> +  case Instruction::Mul: {       // (icmp pred (mul X, Val), CI)
> +    ConstantInt *Val = dyn_cast<ConstantInt>(LHSI->getOperand(1));
> +    if (!Val) break;
> +
> +    if (!ICI.isEquality()) {

Is this check needed?  Won't the isSignTest check take care of it?  If not, it
probably should.

> +      // If this is a signed comparison to 0 and the mul is sign preserving,
> +      // use the mul LHS operand instead.
> +      ICmpInst::Predicate pred = ICI.getPredicate();
> +      if (isSignTest(pred, RHS) && !Val->isZero() &&
> +          cast<BinaryOperator>(LHSI)->hasNoSignedWrap())
> +          return new ICmpInst(Val->isNegative() ?
> +                                  ICmpInst::getSwappedPredicate(pred) : pred,
> +                              LHSI->getOperand(0),
> +                              Constant::getNullValue(RHS->getType()));
> +    }
> +
> +    break;
> +  }
> +
>     case Instruction::Shl: {       // (icmp pred (shl X, ShAmt), CI)
>       ConstantInt *ShAmt = dyn_cast<ConstantInt>(LHSI->getOperand(1));
>       if (!ShAmt) break;
> @@ -1541,6 +1611,19 @@ Instruction *InstCombiner::visitICmpInst
>               return new ICmpInst(pred, X, NegX);
>             }
>           }
> +        break;
> +      case Instruction::Mul:
> +        if (RHSV == 0) {
> +          if (ConstantInt *BOC = dyn_cast<ConstantInt>(BO->getOperand(1))) {
> +            // The trivial case (mul X, 0) is handled by InstSimplify
> +            // General case : (mul X, C) != 0 iff X != 0
> +            //                (mul X, C) == 0 iff X == 0
> +            if (!BOC->isZero())
> +              return new ICmpInst(ICI.getPredicate(), BO->getOperand(0),
> +                                  Constant::getNullValue(RHS->getType()));

Looks like you forgot to check for nsw here.

Ciao, Duncan.