[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/

[Dan Gohman]

On Feb 12, 2008, at 10:50 PM, Chris Lattner wrote:

> On Feb 12, 2008, at 4:35 PM, Dan Gohman wrote:
>>
>> +++ 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.

Ok.

>
>
>>  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);

Ok.

>
>
>>  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.

Those are "bang bang" operators, which are quite obvious once you're
used to the idiom :-).  I'll change them if you really don't like them.

>
>
> 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.

I don't see a way to do this in a single step with the current APInt
API. And this doesn't seem common enough to want a
specialized API call.

>
>
>> @@ -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?

Ok.

>
>
>>    // 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.

See above.

>
>
>>  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.

Ok.

>
>
>>  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.

It's actually right, though the code is more involved than it needs to  
be.
I simplified this.

>
>
>> @@ -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. :)

Ok, it does now.

>
>
>> @@ -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.

Such a method already existed, but was erroneously named isPositve. I've
fixed that now.

>
>
> 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.

In the interest of keeping client code simpler, I'd prefer to avoid  
having
a separate vector version.

Dan