[llvm-commits] CVS: llvm/lib/Transforms/Scalar/InstructionCombining.cpp

[Chris Lattner]

On Mar 23, 2007, at 11:46 AM, Reid Spencer wrote:
>    // shl uint X, 32 = 0 and shr ubyte Y, 9 = 0, ... just don't  
> eliminate shr
>    // of a signed value.
>    //
> -  unsigned TypeBits = Op0->getType()->getPrimitiveSizeInBits();
> -  if (Op1->getZExtValue() >= TypeBits) {
> +  if (Op1->getZExtValue() >= TypeBits) {  // shift amount always  
> <= 32 bits

This isn't safe.  Code like:

shl i1024 %A, 123456789123456789123456789123456789

is undefined but legal.  The compiler should not crash on it.

> @@ -6462,8 +6460,8 @@
>          // operation.
>          //
>          if (isValid && !isLeftShift && I.getOpcode() ==  
> Instruction::AShr) {
> -          uint64_t Val = Op0C->getZExtValue();
> -          isValid = ((Val & (1 << (TypeBits-1))) != 0) == highBitSet;
> +          APInt Val(Op0C->getValue());
> +          isValid = ((Val & APInt::getSignBit(TypeBits)) != 0) ==  
> highBitSet;
>          }

This should use operator[] to get the sign bit, or use isPositive/ 
isNegative.
>
>          if (isValid) {
> @@ -6488,6 +6486,7 @@
>
>    if (ShiftOp && isa<ConstantInt>(ShiftOp->getOperand(1))) {
>      ConstantInt *ShiftAmt1C = cast<ConstantInt>(ShiftOp->getOperand 
> (1));
> +    // shift amount always <= 32 bits

Likewise, not safe.

>      unsigned ShiftAmt1 = (unsigned)ShiftAmt1C->getZExtValue();
>      unsigned ShiftAmt2 = (unsigned)Op1->getZExtValue();
>      assert(ShiftAmt2 != 0 && "Should have been simplified earlier");
> @@ -6515,8 +6514,8 @@
>          BinaryOperator::createAShr(X, ConstantInt::get(Ty, AmtSum));
>        InsertNewInstBefore(Shift, I);
>
> -      uint64_t Mask = Ty->getBitMask() >> ShiftAmt2;
> -      return BinaryOperator::createAnd(Shift, ConstantInt::get(Ty,  
> Mask));
> +      APInt Mask(APInt::getAllOnesValue(TypeBits).lshr(ShiftAmt2));
> +      return BinaryOperator::createAnd(Shift, ConstantInt::get 
> (Mask));
>      }

One of many places that need to use the new methods.

> @@ -6538,9 +6537,12 @@
>        // generators.
>        const Type *SExtType = 0;
>        switch (Ty->getBitWidth() - ShiftAmt1) {
> -      case 8 : SExtType = Type::Int8Ty; break;
> -      case 16: SExtType = Type::Int16Ty; break;
> -      case 32: SExtType = Type::Int32Ty; break;
> +      case 1  : SExtType = Type::Int1Ty; break;
> +      case 8  : SExtType = Type::Int8Ty; break;
> +      case 16 : SExtType = Type::Int16Ty; break;
> +      case 32 : SExtType = Type::Int32Ty; break;
> +      case 64 : SExtType = Type::Int64Ty; break;
> +      case 128: SExtType = IntegerType::get(128); break;
>        default: break;

It would be more efficient (code size) to do:

        switch (Ty->getBitWidth() - ShiftAmt1) {
        case 1:
        case 8:
        case 16:
        case 32:
        case 64:
        case 128:
          SExtType = IntegerType::get(Ty->getBitWidth() - ShiftAmt1);
          break;
        default: break;


> @@ -8191,7 +8193,7 @@
>        (ParamTy->isInteger() && ActTy->isInteger() &&
>         ParamTy->getPrimitiveSizeInBits() >= ActTy- 
> >getPrimitiveSizeInBits()) ||
>        (c && ParamTy->getPrimitiveSizeInBits() >= ActTy- 
> >getPrimitiveSizeInBits()
> -       && c->getSExtValue() > 0);
> +       && c->getValue().isPositive());

Doesn't isPositive return true for zero also?  If so, this is a bug.

-Chris