[llvm-commits] [llvm] r47039 - in /llvm/trunk: include/llvm/CodeGen/ include/llvm/Target/ lib/CodeGen/SelectionDAG/ lib/Target/ARM/ lib/Target/CellSPU/ lib/Target/PowerPC/ lib/Target/Sparc/ lib/Target/X86/

[Chris Lattner]

On Feb 12, 2008, at 4:35 PM, Dan Gohman wrote:
> Convert SelectionDAG::ComputeMaskedBits to use APInt instead of  
> uint64_t.
> Add an overload that supports the uint64_t interface for use by  
> clients
> that haven't been updated yet.

Great!  Thanks for tackling this Dan!

> +++ llvm/trunk/include/llvm/CodeGen/SelectionDAG.h Tue Feb 12  
> 18:35:47 2008
> @@ -556,6 +556,12 @@
>   /// bitsets.  This code only analyzes bits in Mask, in order to  
> short-circuit
>   /// processing.  Targets can implement the  
> computeMaskedBitsForTargetNode
>   /// method in the TargetLowering class to allow target nodes to be  
> understood.
> +  void ComputeMaskedBits(SDOperand Op, APInt Mask, APInt &KnownZero,

Please pass Mask by const reference (likewise to the virtual method).   
APInts are somewhat cheap to copy in the common case (no malloc) but  
they aren't free, and it would be better to make the copies explicit  
so they are only done when needed.

>   case ISD::SRL:
>     // (ushr X, C1) & C2 == 0   iff  (-1 >> C1) & C2 == 0
..

> +      APInt HighBits = APInt::getHighBitsSet(BitWidth, ShAmt);
>       KnownZero |= HighBits;  // High bits known zero.

How about:  KnownZero |= APInt::getHighBitsSet(BitWidth, ShAmt);

>   case ISD::SRA:
>     if (ConstantSDNode *SA =  
> dyn_cast<ConstantSDNode>(Op.getOperand(1))) {
>       unsigned ShAmt = SA->getValue();
>
> +      APInt InDemandedMask = (Mask << ShAmt);
>       // If any of the demanded bits are produced by the sign  
> extension, we also
>       // demand the input sign bit.
> +      APInt HighBits = APInt::getHighBitsSet(BitWidth, ShAmt);
> +      if (!!(HighBits & Mask))
> +        InDemandedMask |= APInt::getSignBit(BitWidth);

Heh, this is clever, but non-obvious.  How about:

   if ((HighBits & Mask) != 0)

or:
   if ((HighBits & Mask).getBoolValue())

which is less tricky but more obvious.  There are a couple instances  
of similar !! things.

In fact, this occurs often enough that it might be worthwhile to avoid  
the construction of the temporary APInt.  How about defining a new  
method:

   X.intersect(Y)

to be true if any bits in X are also set in Y?  Aka (X&Y)!=0


>       ComputeMaskedBits(Op.getOperand(0), InDemandedMask, KnownZero,  
> KnownOne,
>                         Depth+1);
>       assert((KnownZero & KnownOne) == 0 && "Bits known to be one  
> AND zero?");
> +      KnownZero = KnownZero.lshr(ShAmt);
> +      KnownOne  = KnownOne.lshr(ShAmt);
>
>       // Handle the sign bits.
> +      APInt SignBit = APInt::getSignBit(BitWidth);
> +      SignBit = SignBit.lshr(ShAmt);  // Adjust to where it is now  
> in the mask.

It would be better to make an APInt with the bit in the right place  
instead of starting with a sign bit and shifting it.

> @@ -1283,14 +1274,18 @@
>
>     // Sign extension.  Compute the demanded bits in the result that  
> are not
>     // present in the input.
> -    uint64_t NewBits = ~MVT::getIntVTBitMask(EVT) & Mask;
> +    APInt NewBits = ~APInt::getLowBitsSet(BitWidth,
> +                                          MVT::getSizeInBits(EVT))  
> & Mask;

instead of using ~lowbits, how about using getHighBitsSet?

>     // If the sign extended bits are demanded, we know that the sign
>     // bit is demanded.
> +    InSignBit.zext(BitWidth);

This is another "create sign and extend".  It would be better to just  
make the apint with the bit set in the right place.

>   case ISD::LOAD: {
>     if (ISD::isZEXTLoad(Op.Val)) {
>       LoadSDNode *LD = cast<LoadSDNode>(Op);
>       MVT::ValueType VT = LD->getMemoryVT();
> -      KnownZero |= ~MVT::getIntVTBitMask(VT) & Mask;
> +      KnownZero |= ~APInt::getLowBitsSet(BitWidth,  
> MVT::getSizeInBits(VT)) & Mask;

Instead of ~lowbits, how about using highbits?  likewise in a couple  
other places.

>   case ISD::SIGN_EXTEND: {

>     MVT::ValueType InVT = Op.getOperand(0).getValueType();
> +    unsigned InBits = MVT::getSizeInBits(InVT);
> +    APInt InMask    = APInt::getLowBitsSet(BitWidth, InBits);
> +    APInt InSignBit = APInt::getSignBit(InBits);
> +    APInt NewBits   = (~InMask) & Mask;
>
>     // If any of the sign extended bits are demanded, we know that  
> the sign
>     // bit is demanded.
> +    InSignBit.zext(BitWidth);
> +    if (!!(NewBits & Mask))
> +      Mask |= InSignBit;

I think this is a bug: NewBits is defined to be "... & Mask".  This is  
either a bug or this can be replaced with "NewBits != 0" which doesn't  
seem right.

> @@ -1402,11 +1415,11 @@
>     // Output known-0 bits are known if clear or set in both the low  
> clear bits
>     // common to both LHS & RHS.  For example, 8+(X<<3) is known to  
> have the
>     // low 3 bits clear.
> -    uint64_t KnownZeroOut =  
> std::min(CountTrailingZeros_64(~KnownZero),
> -                                      
> CountTrailingZeros_64(~KnownZero2));
> +    unsigned KnownZeroOut =  
> std::min((~KnownZero).countTrailingZeros(),
> +                                      
> (~KnownZero2).countTrailingZeros());

Huh, apint has a countLeadingOnes() but no countTrailingOnes().  If it  
did, you could use it instead of ~'ing the apint. :)

> @@ -1416,21 +1429,23 @@
>     // We know that the top bits of C-X are clear if X contains less  
> bits
>     // than C (i.e. no wrap-around can happen).  For example, 20-X is
>     // positive if we can prove that X is >= 0 and < 16.
> +
> +    // sign bit clear
> +    if (!(CLHS->getAPIntValue() & APInt::getSignBit(BitWidth))) {

How about: CLHS->getAPIntValue()[BitWidth-1] != 0

it would be even nicer to add an APInt::getSignBit() non-static method  
that returned the signbit directly as a bool.

Incidentally, I think that this method:

  SDOperand SelectionDAG::getConstant(const APInt &Val, MVT::ValueType  
VT, bool isTarget = false);

Can be simplified to:

  SDOperand SelectionDAG::getConstant(const APInt &Val, bool isTarget  
= false);

And have it infer the MVT from the width of the APInt.  We can add a  
new (explicit) method to handle the vector case.

-Chris