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

[Arnaud A. de Grandmaison]

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


-- 
Arnaud A. de Grandmaison

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Address-issues-found-by-Duncan-during-post-commit-re.patch
Type: text/x-patch
Size: 4501 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130325/e974a480/attachment.bin>