Hi Chris,
Feedback below ...
On Tue, 2006-12-12 at 22:42 -0800, Chris Lattner wrote:
> This code (around line 2173):
>
> CastInst *BoolCast = 0;
> if (CastInst *CI = dyn_cast<CastInst>(I.getOperand(0)))
> if (CI->getOperand(0)->getType() == Type::BoolTy)
> BoolCast = CI;
> if (!BoolCast)
> if (CastInst *CI = dyn_cast<CastInst>(I.getOperand(1)))
> if (CI->getOperand(0)->getType() == Type::BoolTy)
> BoolCast = CI;
>
> Should *only* allow zext from bool (not any cast). This is a serious
> bug from the original cast patch.
Committed last night.
>
>
> > @@ -2190,7 +2193,7 @@
> > SCOpTy-
> > >getPrimitiveSizeInBits()-1);
> > if (SCIOp0->getType()->isUnsigned()) {
> > const Type *NewTy = SCIOp0->getType()->getSignedVersion();
> > - SCIOp0 = InsertCastBefore(SCIOp0, NewTy, I);
> > + SCIOp0 = InsertCastBefore(Instruction::BitCast, SCIOp0,
> > NewTy, I);
> > }
>
> This cast be removed now that ashr is signless.
Right. Fixed. When SETCC finally gets in, I'll be looking for all kinds
of useless casts, and not only in InstCombine.
>
>
> > @@ -2863,12 +2872,14 @@
> > Constant *ShrMask = ConstantExpr::getLShr(AllOne, OpRHS);
> > Constant *CI = ConstantExpr::getAnd(AndRHS, ShrMask);
> > if (CI == AndRHS) { // Masking out bits shifted in.
> > + // (Val ashr C1) & C2 -> (Val lshr C1) & C2
> > // Make the argument unsigned.
> > Value *ShVal = Op->getOperand(0);
> > ShVal = InsertNewInstBefore(new ShiftInst
> > (Instruction::LShr, ShVal,
> > OpRHS, Op-
> > >getName()),
> > TheAnd);
> > - Value *AndRHS2 = ConstantExpr::getCast(AndRHS, ShVal-
> > >getType());
> > + Value *AndRHS2 = ConstantExpr::getBitCast(AndRHS, ShVal-
> > >getType());
> > +
>
> This cast looks completely dead, if so, it should just be removed.
Removed.
>
> > @@ -2897,9 +2908,9 @@
> > InsertNewInstBefore(Add, IB);
> > // Convert to unsigned for the comparison.
> > const Type *UnsType = Add->getType()->getUnsignedVersion();
> > - Value *OffsetVal = InsertCastBefore(Add, UnsType, IB);
> > + Value *OffsetVal = InsertCastBefore(Instruction::BitCast, Add,
> > UnsType, IB);
> > AddCST = ConstantExpr::getAdd(AddCST, Hi);
> > - AddCST = ConstantExpr::getCast(AddCST, UnsType);
> > + AddCST = ConstantExpr::getBitCast(AddCST, UnsType);
> > return new SetCondInst(Instruction::SetLT, OffsetVal, AddCST);
>
> If only we had signless comparisons! :-)
Indeed. I'd like to get back to finishing it off :)
>
> > @@ -3917,11 +3930,11 @@
> > for (unsigned i = 1, e = GEP->getNumOperands(); i != e; ++i, +
> > +GTI) {
> > Value *Op = GEP->getOperand(i);
> > uint64_t Size = TD.getTypeSize(GTI.getIndexedType()) &
> > PtrSizeMask;
> > - Constant *Scale = ConstantExpr::getCast(ConstantInt::get
> > (UIntPtrTy, Size),
> > + Constant *Scale = ConstantExpr::getBitCast(ConstantInt::get
> > (UIntPtrTy, Size),
> > SIntPtrTy);
>
> This doesn't fit in 80 columns.
Fixed.
> Further, you can just use:
>
> > Constant *Scale = ConstantInt::get(SIntPtrTy, Size);
>
> Now that Constant[SU]Int got merged into ConstantInt.
Yup. Done.
>
>
> This code:
> // Check to see if there is a noop-cast between the shift
> and the and.
> if (!Shift) {
> if (CastInst *CI = dyn_cast<CastInst>(LHSI->getOperand(0)))
> if (CI->getOperand(0)->getType()->isIntegral() &&
> CI->getOperand(0)->getType()-
> >getPrimitiveSizeInBits() ==
> CI->getType()->getPrimitiveSizeInBits())
> Shift = dyn_cast<ShiftInst>(CI->getOperand(0));
> }
>
> Should check for bitcast with integral src/dest instead of checking
> sizes etc.
Done.
>
>
> This code:
>
> // Make sure we insert a logical shift.
> Constant *NewAndCST = AndCST;
> if (AndCST->getType()->isSigned())
> NewAndCST = ConstantExpr::getBitCast(AndCST,
> AndCST->getType()-
> >getUnsignedVersion());
> NS = new ShiftInst(Instruction::LShr, NewAndCST,
> Shift->getOperand(1), "tmp");
>
> You can drop the cast now that the shift is signless.
Yup. Done.
>
>
> The two casts here can be eliminated now that shifts are signless:
>
> // (X >>s C1) << C2 where C1 > C2 === (X >>s (C1-C2)) & mask
> Op = InsertCastBefore(Instruction::BitCast, Mask,
> I.getType()->getSignedVersion(), I);
> Instruction *Shift =
> new ShiftInst(ShiftOp->getOpcode(), Op,
> ConstantInt::get(Type::UByteTy, ShiftAmt1-
> ShiftAmt2));
> InsertNewInstBefore(Shift, I);
>
> C = ConstantIntegral::getAllOnesValue(Shift->getType());
> C = ConstantExpr::getShl(C, Op1);
> Mask = BinaryOperator::createAnd(Shift, C, Op->getName()
> +".mask");
> InsertNewInstBefore(Mask, I);
> return CastInst::create(Instruction::BitCast, Mask, I.getType
> ());
Done.
>
>
> > @@ -5874,10 +5893,15 @@
> > if (DestBitSize == SrcBitSize ||
> > !ValueRequiresCast(Op1, DestTy,TD) ||
> > !ValueRequiresCast(Op0, DestTy, TD)) {
> > - Value *Op0c = InsertOperandCastBefore(Op0, DestTy, SrcI);
> > - Value *Op1c = InsertOperandCastBefore(Op1, DestTy, SrcI);
> > - return BinaryOperator::create(cast<BinaryOperator>(SrcI)
> > - ->getOpcode(), Op0c, Op1c);
> > + unsigned Op0BitSize = Op0->getType()-
> > >getPrimitiveSizeInBits();
> > + Instruction::CastOps opcode =
> > + (Op0BitSize > DestBitSize ? Instruction::Trunc :
> > + (Op0BitSize == DestBitSize ? Instruction::BitCast :
> > + Op0->getType()->isSigned() ?
> > Instruction::SExt :Instruction::ZExt));
>
> This is a serious bug. You can't look at the type of the operand to
> determine the type of the cast! Replace this with:
>
> Instruction::CastOps opcode = CI.getOpcode();
Done.
>
> > @@ -6014,13 +6042,18 @@
> >
> > // Get a mask for the bits shifting in.
> > uint64_t Mask = (~0ULL >> (64-ShAmt)) << DestBitWidth;
> > - if (SrcI->hasOneUse() && MaskedValueIsZero(SrcI->getOperand
> > (0), Mask)) {
> > + Value* SrcIOp0 = SrcI->getOperand(0);
> > + if (SrcI->hasOneUse() && MaskedValueIsZero(SrcIOp0, Mask)) {
> > if (ShAmt >= DestBitWidth) // All zeros.
> > return ReplaceInstUsesWith(CI, Constant::getNullValue
> > (Ty));
> >
> > // Okay, we can shrink this. Truncate the input, then
> > return a new
> > // shift.
> > - Value *V = InsertCastBefore(SrcI->getOperand(0), Ty, CI);
> > + Instruction::CastOps opcode =
> > + (SrcIOp0->getType()->getPrimitiveSizeInBits() ==
> > + Ty->getPrimitiveSizeInBits() ? Instruction::BitCast :
> > + Instruction::Trunc);
>
> This is always trunc.
>
>
> > @@ -7316,15 +7355,18 @@
> > return 0;
> > }
> >
> > -static Value *InsertSignExtendToPtrTy(Value *V, const Type *DTy,
> > - Instruction *InsertPoint,
> > - InstCombiner *IC) {
> > - unsigned PS = IC->getTargetData().getPointerSize();
> > - const Type *VTy = V->getType();
> > - if (!VTy->isSigned() && VTy->getPrimitiveSize() < PS)
> > - // We must insert a cast to ensure we sign-extend.
> > - V = IC->InsertCastBefore(V, VTy->getSignedVersion(),
> > *InsertPoint);
> > - return IC->InsertCastBefore(V, DTy, *InsertPoint);
> > +static Value *InsertCastToIntPtrTy(Value *V, const Type *DTy,
> > + Instruction *InsertPoint,
> > + InstCombiner *IC) {
>
> Why did you rename this?
Because the name doesn't match its action. SExt is only one possibility.
The type of Value* can be larger, smaller, or equal in size to DTy so it
can be SExt, Trunc, or BitCast. I figured
InsertSignExtendOrTruncateOrBitCastToPtrTy was a bit long.
> The previous name was better (indicated
> sign extension).
The previous name is misleading. It doesn't always do a SExt. Consider
long GEP index on a 32-bit platform. The cast needed is a Trunc.
>
> > + unsigned PtrSize = IC->getTargetData().getPointerSize();
>
> This isn't right. Two callers pass in intptr_t, but two pass in
> something else. You should use DTy->getPrimitiveSize() instead of
> TD.getPointerSize()
Okay, missed that. Thanks.
>
> > + unsigned VTySize = V->getType()->getPrimitiveSize();
> > + // We must cast correctly to the pointer type. Ensure that we
> > + // sign extend the integer value if it is smaller as this is
> > + // used for address computation.
> > + Instruction::CastOps opcode =
> > + (VTySize < PtrSize ? Instruction::SExt :
> > + (VTySize == PtrSize ? Instruction::BitCast :
> > Instruction::Trunc));
> > + return IC->InsertCastBefore(opcode, V, DTy, *InsertPoint);
> > }
>
>
> > @@ -8677,8 +8721,8 @@
> > std::vector<Value*> Ops(CE->op_begin()+1, CE->op_end());
> > uint64_t Offset = TD->getIndexedOffset(Ptr->getType(), Ops);
> > Constant *C = ConstantInt::get(Type::ULongTy, Offset);
> > + C = ConstantExpr::getIntegerCast(C, TD->getIntPtrType(),
> > true /*SExt*/);
>
> Just use:
>
> > Constant *C = ConstantInt::get(TD->getIntPtrType(), Offset);
Done.
>
> -Chris
>
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
