[llvm-commits] [llvm] r128732 - in /llvm/trunk: lib/Transforms/InstCombine/InstCombineCasts.cpp test/Transforms/InstCombine/sext.ll

[Benjamin Kramer]

On 01.04.2011, at 23:46, Frits van Bommel wrote:

> On Fri, Apr 1, 2011 at 10:09 PM, Benjamin Kramer
> <benny.kra at googlemail.com> wrote:
>> +  // Transforming icmps with more than one use is not profitable.
>> +  if (!ICI->hasOneUse())
>> +    return 0;
> 
> Did you test this?
> Intuitively, independent icmp + ashr could be more efficient than a
> (sext (icmp)) with extra uses for the icmp. Then again, they may not
> be since they introduce an extra use of 'x'.
> Just a thought.

sext from i1 is typically more expensive than an ashr, so you're probably right. I'll move the use
check to the new optimization, which inserts multiple instructions in the common case.

> 
>> +
>>   if (ConstantInt *Op1C = dyn_cast<ConstantInt>(Op1)) {
>>     // (x <s 0) ? -1 : 0 -> ashr x, 31   -> all ones if signed
>>     // (x >s -1) ? -1 : 0 -> ashr x, 31  -> all ones if not signed
>> @@ -898,6 +902,52 @@
>>         In = Builder->CreateNot(In, In->getName()+".not");
>>       return ReplaceInstUsesWith(CI, In);
>>     }
>> +
>> +    // If we know that only one bit of the LHS of the icmp can be set and we
>> +    // have an equality comparison with zero or a power of 2, we can transform
>> +    // the icmp and sext into bitwise/integer operations.
>> +    if (ICI->isEquality() && (Op1C->isZero() || Op1C->getValue().isPowerOf2())){
>> +      unsigned BitWidth = Op1C->getType()->getBitWidth();
>> +      APInt KnownZero(BitWidth, 0), KnownOne(BitWidth, 0);
>> +      APInt TypeMask(APInt::getAllOnesValue(BitWidth));
>> +      ComputeMaskedBits(Op0, TypeMask, KnownZero, KnownOne);
>> +
>> +      if ((~KnownZero).isPowerOf2()) {
>> +        Value *In = ICI->getOperand(0);
>> +
>> +        if (!Op1C->isZero() == (Pred == ICmpInst::ICMP_NE)) {
>> +          // sext ((x & 2^n) == 0)   -> (x >> n) - 1
>> +          // sext ((x & 2^n) != 2^n) -> (x >> n) - 1
> 
> Did I miss where you checked whether the RHS of the and is equal to
> the RHS of the icmp for the second case?
> Or does this miscompile e.g. sext((x & 8) != 16)?
> (You can optimize that to -1, of course)

That's something that should be constant folded at this point. I'll add an assert to be sure.

> 
>> +          unsigned ShiftAmt = KnownZeroMask.countTrailingZeros();
>> +          // Perform a right shift to place the desired bit in the LSB.
>> +          if (ShiftAmt)
>> +            In = Builder->CreateLShr(In,
>> +                                     ConstantInt::get(In->getType(), ShiftAmt));
>> +
>> +          // At this point "In" is either 1 or 0. Subtract 1 to turn
>> +          // {1, 0} -> {0, -1}.
>> +          In = Builder->CreateAdd(In,
>> +                                  ConstantInt::getAllOnesValue(In->getType()),
>> +                                  "sext");
>> +        } else {
>> +          // sext ((x & 2^n) != 0)   -> (x << bitwidth-n) a>> bitwidth-1
>> +          // sext ((x & 2^n) != 2^n) -> (x << bitwidth-n) a>> bitwidth-1
> 
> I think you meant '==' in the second case.
> And this seems to have the same issue as above, miscompiling 'sext((x
> & 8) == 16)' (which is 0).

Doh! Stupid copy and paste.

> 
>> +          unsigned ShiftAmt = KnownZeroMask.countLeadingZeros();
>> +          // Perform a left shift to place the desired bit in the MSB.
>> +          if (ShiftAmt)
>> +            In = Builder->CreateShl(In,
>> +                                    ConstantInt::get(In->getType(), ShiftAmt));
>> +
>> +          // Distribute the bit over the whole bit width.
>> +          In = Builder->CreateAShr(In, ConstantInt::get(In->getType(),
>> +                                                        BitWidth - 1), "sext");
>> +        }
>> +
>> +        if (CI.getType() == In->getType())
>> +          return ReplaceInstUsesWith(CI, In);
>> +        return CastInst::CreateIntegerCast(In, CI.getType(), true/*SExt*/);
>> +      }
>> +    }
>>   }