[llvm-commits] CVS: llvm/lib/VMCore/ConstantFold.cpp

[Chris Lattner]

On Feb 26, 2007, at 10:24 PM, Reid Spencer wrote:

>    case Instruction::FPToUI:
> -    if (const ConstantFP *FPC = dyn_cast<ConstantFP>(V))
> -      return ConstantInt::get(DestTy,(uint64_t) FPC->getValue());
> +    if (const ConstantFP *FPC = dyn_cast<ConstantFP>(V)) {
> +      APInt Val(APIntOps::RoundDoubleToAPInt(FPC->getValue()));
> +      uint32_t DestBitWidth = cast<IntegerType>(DestTy)- 
> >getBitWidth();
> +      if (Val.getBitWidth() > DestBitWidth)
> +        Val.trunc(DestBitWidth);

This seems like a poor choice of API.  RoundDoubleToAPInt should take  
a result bitwidth.  Without this, this appears like it could make a  
"large" APInt, then truncate it down to a small one when converting  
from double, which is very inefficient.

> @@ -191,23 +206,37 @@
>      return 0;                   // Other pointer types cannot be  
> casted
>    case Instruction::UIToFP:
>      if (const ConstantInt *CI = dyn_cast<ConstantInt>(V))
> -      return ConstantFP::get(DestTy, double(CI->getZExtValue()));
> +      if (CI->getType()->getBitWidth() <= APInt::APINT_BITS_PER_WORD)
> +        return ConstantFP::get(DestTy, CI->getValue().roundToDouble 
> (false));

Why the check for APINT_BITS_PER_WORD here?  APINT_BITS_PER_WORD is  
an implementation detail of APInt, not a part of its public  
interface.  Also, please split the roundToDouble method into two  
methods which don't take a bool.

>      return 0;
>    case Instruction::SIToFP:
>      if (const ConstantInt *CI = dyn_cast<ConstantInt>(V))
> -      return ConstantFP::get(DestTy, double(CI->getSExtValue()));
> +      if (CI->getType()->getBitWidth() <= APInt::APINT_BITS_PER_WORD)
> +        return ConstantFP::get(DestTy, CI->getValue().roundToDouble 
> (true));
>      return 0;

Likewise.

>
> @@ -382,7 +412,7 @@
>      Ops.reserve(numOps);
>      for (unsigned i = 0; i < numOps; ++i) {
>        const Constant *Op =
> -        (i == idxVal) ? Elt : Constant::getNullValue(Elt->getType());
> +        (idxVal == i) ? Elt : Constant::getNullValue(Elt->getType());

This sort of change makes the diff more noisy, and there is no  
apparent reason for doing it.  Generally, the mutating value (i) is  
on the LHS, why do this (in many places)?


> @@ -554,56 +585,66 @@
>        case Instruction::SDiv:
> +        if (CI2->isNullValue())
> +          return 0;        // X / 0 -> can't fold
> +        return ConstantInt::get(C1->getType(), C1V.sdiv(C2V));
> +        if (C2V.isAllOnesValue() && C1V.isMinSignedValue())
> +          return 0;        // MIN_INT / -1 -> overflow
> +        return ConstantInt::get(C1->getType(), C1V.sdiv(C2V));

This isn't right, remove the first return.

>        case Instruction::Shl:
> -        return ConstantInt::get(C1->getType(), C1Val << C2Val);
> +        if (uint32_t shiftAmt = C2V.getZExtValue())
> +          if (shiftAmt <= C1V.getBitWidth())

This should be <, not <=, likewise for other shifts.

-Chris