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
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
