[llvm-commits] [llvm] r89639 - in /llvm/trunk: lib/Transforms/Scalar/InstructionCombining.cpp test/Transforms/InstCombine/compare-signs.ll

[Nick Lewycky]

Chris Lattner wrote:
>
> On Nov 22, 2009, at 7:17 PM, Nick Lewycky wrote:
>
>> Author: nicholas
>> Date: Sun Nov 22 21:17:33 2009
>> New Revision: 89639
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=89639&view=rev
>> Log:
>> Reapply r88830 with a bugfix: this transform only applies to icmp eq/ne. This
>> fixes part of PR5438.
>
> Thanks for working on this, some thoughts:
>
>>
>> +  // icmp ne A, B is equal to xor A, B when A and B only really have one bit.
>> +  // It is also profitable to transform icmp eq into not(xor(A, B)) because that
>> +  // may lead to additional simplifications.
>> +  if (ICI->isEquality()&&  CI.getType() == ICI->getOperand(0)->getType()) {
>> +    if (const IntegerType *ITy = dyn_cast<IntegerType>(CI.getType())) {
>> +      uint32_t BitWidth = ITy->getBitWidth();
>> +      if (BitWidth>  1) {
>
> If this is a zext from bool, the result type can't be i1.  The source and dest of a zext can't be the same type, so this 'if' seems unneeded.

Good point. Removed!

>> +        Value *LHS = ICI->getOperand(0);
>> +        Value *RHS = ICI->getOperand(1);
>> +
>> +        APInt KnownZeroLHS(BitWidth, 0), KnownOneLHS(BitWidth, 0);
>> +        APInt KnownZeroRHS(BitWidth, 0), KnownOneRHS(BitWidth, 0);
>> +        APInt TypeMask(APInt::getHighBitsSet(BitWidth, BitWidth-1));
>
> Why isn't TypeMask looking at all the bits?

Does now!

>> +        ComputeMaskedBits(LHS, TypeMask, KnownZeroLHS, KnownOneLHS);
>> +        ComputeMaskedBits(RHS, TypeMask, KnownZeroRHS, KnownOneRHS);
>> +
>> +        if (KnownZeroLHS.countLeadingOnes() == BitWidth-1&&
>> +            KnownZeroRHS.countLeadingOnes() == BitWidth-1) {
>
> This check seems over-constrained.  This xforms should apply when it is one of *any* bit set.  For example, this:

True. Note though that worst case we turn zext+icmp into xor+lshr+and+xor.

> entry:
>          %0 = and i32 %a, 8            ;<i32>  [#uses=1]
>          %1 = and i32 %b, 8            ;<i32>  [#uses=1]
>          %2 = icmp eq i32 %0, %1         ;<i1>  [#uses=1]
>          %3 = zext i1 %2 to i32          ;<i32>  [#uses=1]
>          ret i32 %3
> }
>
> should also be transformed.

No. Instcombine does Other Things to this IR before it ever makes it to 
transformZExtICmp. I've added another testcase which does match the 
transformation you asked for.

Nick