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

[Duncan Sands]

Hi Arnaud, LGTM.

Ciao, Duncan.

On 25/03/13 12:31, Arnaud A. de Grandmaison wrote:
> On 03/25/2013 11:22 AM, Duncan Sands wrote:
>> Hi Arnaud,
>>
>
> Hi Duncan,
>
>>> --- 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.
>>
>
> Fixed.
>
>>> +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.
>>
>
> I was not aware of this invariant. I removed those cases and the tests
> still passes, so the invariant seems to hold :) This does simplify the
> routine.
>
>>> +      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.
>
> I addressed this in the rework of isSignTest.
>
>>
>>> +      // 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.
>
> Fixed.
>
>>
>> Ciao, Duncan.
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>