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