[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 13, 2008, at 3:25 PM, Dan Gohman wrote:
>>>     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.

One way to do it would be:

APInt SignBit(BitWidth, 0);
SignBit.set(position);

It's not significantly less code, but it is more efficient :)

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

Thanks!

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

ok, well, the client code would be even simpler if we had two  
versions: one that takes an apint, and one that takes an apint + MVT.   
The first could forward to the second, and be used in cases where the  
client knows it is only dealing with scalars, which is quite often.

More generally though: how many places call SelectionDAG::getConstant  
with a potentially vector constant?  It seems a bit strange to me to  
have this, because the client would have to be aware that it is doing  
this and explicitly want it.  It seems like a "getVectorConstant"  
would be more natural (and easier for the reader to understand),  
causing these cases to be explicitly separated.  What do you think?

Thanks Dan!

-Chris