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

[Arnaud A. de Grandmaison]

Committed as r177863.

Thanks Duncan,
--
Arnaud

On 03/25/2013 12:41 PM, Duncan Sands wrote:
> 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
>>
>>
>


-- 
Arnaud A. de Grandmaison