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