[llvm-commits] CAST patch: #5: Instcombine
Reid Spencer
rspencer at reidspencer.com
Tue Nov 21 22:02:18 PST 2006
On Tue, 2006-11-21 at 11:34 -0800, Chris Lattner wrote:
>
> There are some serious bugs here. Please fix them and ask questions
> about whatever doesn't make sense.
Okay.
>
>
> Since I'm going to be out of town shortly, I'm fine with you
> committing this monster patch once:
>
>
> 1. you incorporate all the feedback I have given you or convince me
> I'm wrong about some of it.
> 2. You retest, passing dejagnu and all of llvm-test.
Sure. I wouldn't commit it unless it was 100% clean anyway.
>
>
> Thanks Reid, I'm looking forward to this landing.
Me too!
>
> +/// isCast - If the specified operand is a CastInst or a constant
> expr
> +/// cast, return the operand value, otherwise return null.
> +static Value *getCastOperand(Value *V) {
>
>
> Please update the comment. Further, the only caller of this code is
> interested in bitcasts. Please change this to getBitCastOperand,
> which allows you to delete the !isa<PointerType> case in the caller.
Sure. Done.
>
>
> @@ -1044,78 +1024,112 @@ bool InstCombiner::SimplifyDemandedBits(
>
>
>
> // Only known if known in both the LHS and RHS.
> KnownOne &= KnownOne2;
> KnownZero &= KnownZero2;
> break;
> - case Instruction::Cast: {
> + case Instruction::FPTrunc:
> + case Instruction::FPExt:
> + case Instruction::FPToUI:
> + case Instruction::FPToSI:
> + case Instruction::UIToFP:
> + case Instruction::SIToFP:
> + case Instruction::PtrToInt: {
> + case Instruction::IntToPtr:
>
>
> remove the unhandled cases, just let the default case handle them.
Okay.
>
>
> IntToPtr shouldn't be handled here, this function is never called on
> pointers.
No, but its called on ints and IntToPtr deals with an Int operand.
>
>
>
>
> + case Instruction::BitCast: {
> + // Cast to bool is a comparison against 0, which demands all
> bits. We
> + // can't propagate anything useful up.
> + if (I->getType() == Type::BoolTy)
> + break;
>
>
> Please remove this.
Removed.
>
>
> + case Instruction::Trunc: {
> + // The size of the src must be > the size of the dest and they
> are both
> + // integral types.
> + // Cast to bool is a comparison against 0, which demands all
> bits. We
> + // can't propagate anything useful up.
> + if (I->getType() == Type::BoolTy)
> break;
>
>
> Likewise.
Removed.
>
>
> + // Must be a truncate so just look in the input
>
>
> This comment is not needed any more :)
Removed.
>
>
>
>
> @@ -1617,16 +1631,16 @@ struct AddMaskingAnd {
> }
> };
>
>
>
> static Value *FoldOperationIntoSelectOperand(Instruction &I, Value
> *SO,
> InstCombiner *IC) {
> - if (isa<CastInst>(I)) {
> + if (CastInst *CI = dyn_cast<CastInst>(&I)) {
> if (Constant *SOC = dyn_cast<Constant>(SO))
> - return ConstantExpr::getCast(SOC, I.getType());
> + return ConstantExpr::getCast(CI->getOpcode(), SOC,
> I.getType());
>
>
>
> - return IC->InsertNewInstBefore(new CastInst(SO, I.getType(),
> - SO->getName() +
> ".cast"), I);
> + return IC->InsertNewInstBefore(CastInst::createInferredCast(
> + SO, I.getType(), SO->getName() + ".cast"), I);
> }
>
>
>
> This is a bug. Pass CI->getOpcode() when creating the cast
> instruction instead of using createInferredCast.
Yup, missed that one. Thanks.
>
>
>
> @@ -1737,21 +1751,20 @@ Instruction *InstCombiner::FoldOpIntoPhi
>
>
>
> WorkList.push_back(cast<Instruction>(InV));
> }
> NewPN->addIncoming(InV, PN->getIncomingBlock(i));
> }
> - } else {
> - assert(isa<CastInst>(I) && "Unary op should be a cast!");
> - const Type *RetTy = I.getType();
> + } else if (CastInst *CI = dyn_cast<CastInst>(&I)) {
> + const Type *RetTy = CI->getType();
>
> Please use:
>
>
> } else {
>
> CastInst *CI = cast<CastInst>(I);
> const Type *RetTy = I.getType();
>
> to avoid losing the assertion.
Done.
>
>
> if (Constant *InC =
> dyn_cast<Constant>(PN->getIncomingValue(i))) {
> - InV = ConstantExpr::getCast(InC, RetTy);
> + InV = ConstantExpr::getCast(CI->getOpcode(), InC, RetTy);
> } else {
> assert(PN->getIncomingBlock(i) == NonConstBB);
> - InV = new CastInst(PN->getIncomingValue(i), I.getType(),
> "phitmp",
> - NonConstBB->getTerminator());
> + InV = CastInst::createInferredCast(PN->getIncomingValue(i),
> I.getType(),
> + "phitmp",
> NonConstBB->getTerminator());
> WorkList.push_back(cast<Instruction>(InV));
> }
> NewPN->addIncoming(InV, PN->getIncomingBlock(i));
>
>
> This is a bug. Pass the cast opcode into the cast instruction case.
Yup.
>
>
>
>
>
>
> @@ -2482,15 +2496,16 @@ static Constant *GetFactor(Value *V) {
> unsigned Zeros = CountTrailingZeros_64(RHS->getZExtValue());
> if (Zeros != V->getType()->getPrimitiveSizeInBits())
> return ConstantExpr::getShl(Result,
> ConstantInt::get(Type::UByteTy,
> Zeros));
> }
> - } else if (I->getOpcode() == Instruction::Cast) {
> - Value *Op = I->getOperand(0);
> + } else if (CastInst *CI = dyn_cast<CastInst>(I)) {
> + Value *Op = CI->getOperand(0);
> // Only handle int->int casts.
>
>
> Be more specific: dyn_cast<BitCastInst>
Okay.
>
>
>
>
>
>
> @@ -3122,37 +3137,34 @@ Instruction *InstCombiner::visitAnd(Bina
>
>
>
> if (ConstantInt *Op0CI =
> dyn_cast<ConstantInt>(Op0I->getOperand(1)))
> if (Instruction *Res = OptAndOp(Op0I, Op0CI, AndRHS, I))
> return Res;
> } else if (CastInst *CI = dyn_cast<CastInst>(Op0)) {
> - const Type *SrcTy = CI->getOperand(0)->getType();
> -
> // If this is an integer truncation or change from
> signed-to-unsigned, and
> // if the source is an and/or with immediate, transform it.
> This
> // frequently occurs for bitfield accesses.
> if (Instruction *CastOp =
> dyn_cast<Instruction>(CI->getOperand(0))) {
> - if (SrcTy->getPrimitiveSizeInBits() >=
> - I.getType()->getPrimitiveSizeInBits() &&
> + if ((isa<TruncInst>(CI) ||
> CI->isNoopCast(TD->getIntPtrType())) &&
> CastOp->getNumOperands() == 2)
>
>
>
>
> Do not generalize this to pointers, it will break the code. Please
> use isa<TruncInst> || isa<BitCastInst>
Okay.
>
>
> @@ -4935,20 +4947,20 @@ Instruction *InstCombiner::visitSetCondI
>
>
>
> // Test to see if the operands of the setcc are casted versions of
> other
> // values. If the cast can be stripped off both arguments, we do
> so now.
> if (CastInst *CI = dyn_cast<CastInst>(Op0)) {
> Value *CastOp0 = CI->getOperand(0);
> - if (CastOp0->getType()->isLosslesslyConvertibleTo(CI->getType())
> &&
> - (isa<Constant>(Op1) || isa<CastInst>(Op1)) && I.isEquality())
> {
> + if (CI->isNoopCast(TD->getIntPtrType()) && I.isEquality() &&
> + (isa<Constant>(Op1) || isa<CastInst>(Op1))) {
>
>
> There is no reason to generalize this, stick with isa<BitCast> instead
> of isNoopCast.
>
>
>
>
> // If operand #1 is a cast instruction, see if we can eliminate
> it as
> // well.
> if (CastInst *CI2 = dyn_cast<CastInst>(Op1))
> - if (CI2->getOperand(0)->getType()->isLosslesslyConvertibleTo(
> + if (CI2->getOperand(0)->getType()->canBitCastTo(
>
> Op0->getType()))
>
>
> funky indentation
>
>
>
>
>
>
>
>
> // Find out if this is a shift of a shift by a constant.
> ShiftInst *ShiftOp = 0;
> if (ShiftInst *Op0SI = dyn_cast<ShiftInst>(Op0))
> ShiftOp = Op0SI;
> else if (CastInst *CI = dyn_cast<CastInst>(Op0)) {
> // If this is a noop-integer case of a shift instruction, use the
> shift.
> if (CI->getOperand(0)->getType()->isInteger() &&
> CI->getOperand(0)->getType()->getPrimitiveSizeInBits() ==
> CI->getType()->getPrimitiveSizeInBits() &&
> isa<ShiftInst>(CI->getOperand(0))) {
> ShiftOp = cast<ShiftInst>(CI->getOperand(0));
> }
> }
>
>
>
>
> The first three lines of the inner 'if' should go away once you change
> the dyn_cast<CastInst> to just check for a bitcast.
Yup.
>
>
>
>
> @@ -5399,17 +5413,18 @@ Instruction *InstCombiner::FoldShiftByCo
> if (I.getType() == ShiftResult->getType())
> return ShiftResult;
> InsertNewInstBefore(ShiftResult, I);
> - return new CastInst(ShiftResult, I.getType());
> + return CastInst::createInferredCast(ShiftResult, I.getType());
> }
>
>
> This is always a bitcast.
>
>
>
>
> @@ -5453,11 +5468,11 @@ Instruction *InstCombiner::FoldShiftByCo
>
>
>
> C = ConstantIntegral::getAllOnesValue(Shift->getType());
> C = ConstantExpr::getShl(C, Op1);
> Mask = BinaryOperator::createAnd(Shift, C,
> Op->getName()+".mask");
> InsertNewInstBefore(Mask, I);
> - return new CastInst(Mask, I.getType());
> + return CastInst::createInferredCast(Mask, I.getType());
> }
>
>
> Bitcast
>
>
>
>
>
>
> @@ -5646,11 +5663,22 @@ static bool CanEvaluateInDifferentType(V
> case Instruction::Or:
> case Instruction::Xor:
> // These operators can all arbitrarily be extended or truncated.
> return CanEvaluateInDifferentType(I->getOperand(0), Ty,
> NumCastsRemoved) &&
> CanEvaluateInDifferentType(I->getOperand(1), Ty,
> NumCastsRemoved);
> - case Instruction::Cast:
> + case Instruction::Trunc:
> + case Instruction::ZExt:
> + case Instruction::SExt:
> + case Instruction::FPTrunc:
> + case Instruction::FPExt:
> + case Instruction::FPToUI:
> + case Instruction::FPToSI:
> + case Instruction::UIToFP:
> + case Instruction::SIToFP:
> + case Instruction::IntToPtr:
> + case Instruction::PtrToInt:
> + case Instruction::BitCast:
> // If this is a cast from the destination type, we can trivially
> eliminate
> // it, and this will remove a cast overall.
> if (I->getOperand(0)->getType() == Ty) {
> // If the first operand is itself a cast, and is eliminable, do
> not count
> // this as an eliminable cast. We would prefer to eliminate
> those two
>
>
> The only relevant case here is BitCast. Please remove the others.
Nope. Sorry. This is the source of the test failures. The "Ty" is not
the type of the I, it is the type we hope to cast to. Trunc, ZExt, and
SExt must stay. The others can go because they don't deal with integers.
This code is what handles test cases like:
uint %test30(uint %c1) {
%c2 = trunc uint %c1 to ubyte
%c3 = xor ubyte %c2, 1
%c4 = zext ubyte %c3 to uint
ret uint %c4
}
It allows both casts to be eliminated because the source (uint %c1) and
destination (uint) have the same type. If we always return false for
trunc then these casts don't get eliminated. This is called recursively
and when it gets to the XOR, its asking if the XOR can be computed in
the uint type of the trunc's source to end up at the zext's type, uint.
It can because the source and dest types match.
>
>
> @@ -5686,90 +5716,68 @@ Value *InstCombiner::EvaluateInDifferent
> Value *RHS = EvaluateInDifferentType(I->getOperand(1), Ty);
> Res =
> BinaryOperator::create((Instruction::BinaryOps)I->getOpcode(),
> LHS, RHS, I->getName());
> break;
> }
> - case Instruction::Cast:
> - // If this is a cast from the destination type, return the input.
> + case Instruction::Trunc:
> + case Instruction::ZExt:
> + case Instruction::SExt:
> + case Instruction::FPTrunc:
> + case Instruction::FPExt:
> + case Instruction::FPToUI:
> + case Instruction::FPToSI:
> + case Instruction::UIToFP:
> + case Instruction::SIToFP:
> + case Instruction::IntToPtr:
> + case Instruction::PtrToInt:
> + case Instruction::BitCast:
> + // If the source type of the cast is the type we're trying for
> then we can
> + // just return the source. There's no need to insert it because
> its not new.
> if (I->getOperand(0)->getType() == Ty)
>
>
> likewise.
Same logic. Trunc, ZExt and SExt have to stay. The other's I've deleted.
>
>
>
>
> +/// @brief Implement the transforms common to all CastInst visitors.
> +Instruction *InstCombiner::commonCastTransforms(CastInst &CI) {
> Value *Src = CI.getOperand(0);
>
>
>
> - // If the user is casting a value to the same type, eliminate this
> cast
> - // instruction...
> + // Get rid of casts from one type to the same type. These are
> useless and can
> + // be replaced by the operand.
> if (CI.getType() == Src->getType())
> return ReplaceInstUsesWith(CI, Src);
>
>
> Move this case to the bitcast-specific function.
Okay.
>
>
>
>
>
>
> // If this is a cast to bool, turn it into the appropriate setne
> instruction.
> if (CI.getType() == Type::BoolTy)
> return BinaryOperator::createSetNE(CI.getOperand(0),
>
> Constant::getNullValue(CI.getOperand(0)->getType()));
>
>
> This is a bug, please remove it.
Removed.
>
>
>
>
>
>
> +Instruction *InstCombiner::commonIntCastTransforms(CastInst &CI) {
> ...
> + // See if we can simplify any instructions used by the LHS whose
> sole
> + // purpose is to compute bits we don't care about.
> + if (CI.getType()->isInteger() && Src->getType()->isIntegral()) {
> + uint64_t KnownZero = 0, KnownOne = 0;
> + if (SimplifyDemandedBits(&CI,
> CI.getType()->getIntegralTypeMask(),
> + KnownZero, KnownOne)) {
> + return &CI;
>
>
> No need to check if the cast and operand are integers.
Okay. However, I think we pessimized this slightly from the previous
version. The check in that if statement is "dest is integer, source is
integral". We now only come into this case if both src and dest are
integer. This means that we won't be optimizing any casts from boolean
in here. Whether it makes a difference or not, I'm not sure.
>
>
>
>
> + } else if (Instruction *SrcI = dyn_cast<Instruction>(Src)) {
> + // If the source value is an instruction with only this use, we
> can
> + // attempt to propagate the cast into the instruction.
> + if (SrcI->hasOneUse()) {
>
>
>
>
> Please restructure this to reduce indentation. Eliminating the
> redundant check for integer helps, the next step is to change this
> code to:
>
>
> }
> if (!isa<Instruction>(Src) || !Src->hasOneUse()) return 0;
>
>
> ... which makes the nested if and switch be at the top-level.
Yeah, that makes it much more clear. Done.
>
>
> + case Instruction::PtrToInt:
> + case Instruction::IntToPtr:
>
>
> These cases are dead, you know the src/dst are both integers.
Yup. Deleted.
>
>
>
>
> + if (CI.isNoopCast(TD->getIntPtrType()) ||
> + CI.getOpcode() == Instruction::Trunc)
> + // Just replace this cast with the result.
> + return ReplaceInstUsesWith(CI, Res);
> + switch (CI.getOpcode()) {
>
>
> Change isNoopCast to isa<BitCast>, pointers can't get here. Then
> merge BitCast/Trunc into the switch statement as cases.
Okay.
>
>
>
>
>
>
> + default: assert(0 && "Unknown cast type!");
> + case Instruction::ZExt: {
> + // We need to emit an AND to clear the high bits.
> + unsigned SrcBitSize =
> Src->getType()->getPrimitiveSizeInBits();
> + unsigned DestBitSize =
> CI.getType()->getPrimitiveSizeInBits();
> + assert(SrcBitSize < DestBitSize && "Not a zext?");
> + Constant *C =
> + ConstantInt::get(Type::ULongTy, (1ULL <<
> SrcBitSize)-1);
> + C = ConstantExpr::getCast(C, CI.getType());
>
>
> This ConstantExpr is always a trunc or bitcast.
Yup, fixed to generate the right one based on DestBitSize being < or =
to 64
>
>
> + return BinaryOperator::createAnd(Res, C);
> + }
> + case Instruction::SExt:
> + // We need to emit a cast to truncate, then a cast to
> sext.
> + return CastInst::createInferredCast(
> + InsertCastBefore(Res, Src->getType(), CI),
> CI.getType());
> + }
>
>
> createInferredCast -> Sext
> InsertCastBefore -> trunc
Done.
>
>
>
> + // cast (xor bool X, true) to int --> xor (cast bool X to
> int), 1
> + if (SrcBitSize == 1 && SrcI->getOpcode() ==
> Instruction::Xor &&
> + Op1 == ConstantBool::getTrue() &&
> + (!Op0->hasOneUse() || !isa<SetCondInst>(Op0))) {
> + Value *New = InsertOperandCastBefore(Op0, DestTy, &CI);
> + return BinaryOperator::createXor(New,
> +
> ConstantInt::get(CI.getType(), 1));
> + }
> + break;
>
>
> This isn't safe for sext from bool, plz explicitly check for zext.
Done.
>
>
> + case Instruction::SetEQ:
> + case Instruction::SetNE:
> + // We if we are just checking for a seteq of a single bit
> and casting it
>
>
> We if we are -> If we are
Fixed.
>
>
> + if (isPowerOf2_64(KnownZero^TypeMask)) { // Exactly 1
> possible 1?
> + bool isSetNE = SrcI->getOpcode() ==
> Instruction::SetNE;
> + if (Op1CV && (Op1CV != (KnownZero^TypeMask))) {
> + // (X&4) == 2 --> false
> + // (X&4) != 2 --> true
> + Constant *Res = ConstantBool::get(isSetNE);
> + Res = ConstantExpr::getCast(Res, CI.getType());
> + return ReplaceInstUsesWith(CI, Res);
> + }
>
>
> getCast -> zext
Done.
>
>
>
>
>
>
> +Instruction *InstCombiner::visitTrunc(CastInst &CI) {
> + return this->commonIntCastTransforms(CI);
> +}
>
>
> Why 'this->' ?
No good reason .. removed
>
>
> +Instruction *InstCombiner::visitBitCast(CastInst &CI) {
> +
> + // If the operands are integer typed then apply the integer
> transforms,
> + // otherwise just apply the common ones.
> + if (CI.getType()->isInteger() &&
> CI.getOperand(0)->getType()->isInteger()) {
> + if (Instruction *Result = commonIntCastTransforms(CI))
>
>
>
>
> Only need to check the src or dest type, no need to check both.
Sorry. That will break the assumptions in commonIntCastTransforms that
both src and dest are int typed. don't forget that BitCast can be used
for pointers, packed and float as well.
Not done, to prevent commonIntCastTransforms(CT) from being entered for
things like:
bitcast int %x to float
>
>
> @@ -6107,22 +6251,22 @@ static Constant *GetSelectFoldableConsta
> Instruction *InstCombiner::FoldSelectOpOp(SelectInst &SI, Instruction
> *TI,
> Instruction *FI) {
> if (TI->getNumOperands() == 1) {
> // If this is a non-volatile load or a cast from the same type,
> // merge.
> - if (TI->getOpcode() == Instruction::Cast) {
> + if (TI->isCast()) {
> if (TI->getOperand(0)->getType() !=
> FI->getOperand(0)->getType())
> return 0;
> } else {
> return 0; // unknown unary op.
> }
>
>
>
> // Fold this by inserting a select from the input values.
> SelectInst *NewSI = new SelectInst(SI.getCondition(),
> TI->getOperand(0),
> FI->getOperand(0),
> SI.getName()+".v");
> InsertNewInstBefore(NewSI, SI);
> - return new CastInst(NewSI, TI->getType());
> + return CastInst::createInferredCast(NewSI, TI->getType());
> }
>
>
>
>
> This is a bug. The inserted cast needs to use the same opcode as TI.
Yup. Fixed. That's the third one of these that I missed! You have good
eyes!
>
>
> @@ -6227,17 +6371,17 @@ Instruction *InstCombiner::visitSelectIn
> // Selecting between two integer constants?
> if (ConstantInt *TrueValC = dyn_cast<ConstantInt>(TrueVal))
> if (ConstantInt *FalseValC = dyn_cast<ConstantInt>(FalseVal)) {
> // select C, 1, 0 -> cast C to int
> if (FalseValC->isNullValue() && TrueValC->getZExtValue() == 1)
> {
> - return new CastInst(CondVal, SI.getType());
> + return CastInst::createInferredCast(CondVal, SI.getType());
> } else if (TrueValC->isNullValue() && FalseValC->getZExtValue()
> == 1) {
> // select C, 0, 1 -> cast !C to int
> Value *NotCond =
> InsertNewInstBefore(BinaryOperator::createNot(CondVal,
>
> "not."+CondVal->getName()), SI);
> - return new CastInst(NotCond, SI.getType());
> + return CastInst::createInferredCast(NotCond, SI.getType());
> }
>
>
> These two are zexts.
Yup.
>
>
>
>
> @@ -6271,11 +6415,11 @@ Instruction *InstCombiner::visitSelectIn
>
>
> // The comparison constant and the result are not
> neccessarily the
> // same width. In any case, the first step to do is
> make sure
> // that X is signed.
> Value *X = IC->getOperand(0);
> if (!X->getType()->isSigned())
> X = InsertCastBefore(X,
> X->getType()->getSignedVersion(), SI);
>
>
>
> // Now that X is signed, we have to make the all ones
> value. Do
> // this by inserting a new SRA.
> unsigned Bits = X->getType()->getPrimitiveSizeInBits();
> Constant *ShAmt = ConstantInt::get(Type::UByteTy,
> Bits-1);
> Instruction *SRA = new ShiftInst(Instruction::AShr, X,
> ShAmt, "ones");
> InsertNewInstBefore(SRA, SI);
>
>
>
> // Finally, convert to the type of the select RHS. If
> this is
> // smaller than the compare value, it will truncate the
> ones to
> // fit. If it is larger, it will sext the ones to fit.
> return CastInst::createInferredCast(SRA, SI.getType());
>
>
>
>
> Please eliminate the first cast that makes the operand signed. The
> AShr doesn't need it.
> Once you do that, you won't be able to use createInferredCast.
> Instead, please insert a
> trunc/bitcast/sext as needed.
>
>
Okay. Just decided which to use based on bit size of the operands. Seems
to work :)
>
>
> @@ -6470,12 +6614,11 @@ static unsigned GetKnownAlignment(Value
> Align = std::max(Align,
> (unsigned)TD->getTypeAlignment(Type::LongTy));
> }
> }
> return Align;
> } else if (isa<CastInst>(V) ||
> - (isa<ConstantExpr>(V) &&
> - cast<ConstantExpr>(V)->getOpcode() ==
> Instruction::Cast)) {
> + (isa<ConstantExpr>(V) &&
> cast<ConstantExpr>(V)->isCast())) {
>
>
> Please change this to only permit BitCastInst and BitCast constant
> expr.
Done.
>
>
>
>
> @@ -6666,11 +6809,11 @@ Instruction *InstCombiner::visitCallInst
>
>
>
> // Insert this value into the result vector.
> Result = new InsertElementInst(Result,
> ExtractedElts[Idx], i,"tmp");
> InsertNewInstBefore(cast<Instruction>(Result), CI);
> }
> - return new CastInst(Result, CI.getType());
> + return CastInst::createInferredCast(Result, CI.getType());
>
>
>
> Bitcast always (vector->vector).
Yup. Done.
>
>
>
>
> @@ -6768,11 +6911,11 @@ Instruction *InstCombiner::visitCallSite
> E = CS.arg_end(); I != E; ++I)
> if (CastInst *CI = dyn_cast<CastInst>(*I)) {
> // If this cast does not effect the value passed through the
> varargs
> // area, we can eliminate the use of the cast.
> Value *Op = CI->getOperand(0);
> - if (CI->getType()->isLosslesslyConvertibleTo(Op->getType()))
> {
> + if (CI->getType()->canBitCastTo(Op->getType())) {
> *I = Op;
> Changed = true;
> }
> }
> }
>
> This is wrong. Please change it to isa<BitCast>(CI) && !
> CI->getType()->isFP() && !Op->getType()->isFP()
>
>
> it should not permit fp <-> int bitcasts, but should allow int<->int
> and ptr<->ptr.
What about vector<->vector ?
>
>
> @@ -6784,11 +6927,11 @@ Instruction *InstCombiner::visitCallSite
> // attempt to move the cast to the arguments of the call/invoke.
> //
> bool InstCombiner::transformConstExprCastCall(CallSite CS) {
> if (!isa<ConstantExpr>(CS.getCalledValue())) return false;
> ConstantExpr *CE = cast<ConstantExpr>(CS.getCalledValue());
> - if (CE->getOpcode() != Instruction::Cast || !
> isa<Function>(CE->getOperand(0)))
> + if (!CE->isCast() || !isa<Function>(CE->getOperand(0)))
> return false;
>
> Only permit bitcast (only ptr->ptr is relevant).
>
>
Right. Done.
>
>
> @@ -6799,13 +6942,13 @@ bool InstCombiner::transformConstExprCas
> const Type *OldRetTy = Caller->getType();
>
>
>
> // Check to see if we are changing the return type...
> if (OldRetTy != FT->getReturnType()) {
> if (Callee->isExternal() &&
> - !(OldRetTy->isLosslesslyConvertibleTo(FT->getReturnType()) ||
> + !(OldRetTy->canBitCastTo(FT->getReturnType()) ||
> (isa<PointerType>(FT->getReturnType()) &&
> - TD->getIntPtrType()->isLosslesslyConvertibleTo(OldRetTy)))
> + TD->getIntPtrType()->canBitCastTo(OldRetTy)))
>
> This is wrong. It should not permit fp <-> int bitcasts, but should
> allow int<->int and ptr<->ptr.
canBitCastTo doesn't allow fp <-> int .. that's not losslessly
convertible.
In retrospect it seems that canBitCastTo is not an identity with BitCast
and the name isLosslesslyConvertibleTo is a better name. The reason is
that the function doesn't allow you to translate between fundamental
types (e.g. int <-> fp, int <-> packed, etc.) while the bitcast
instruction does. The only requirements on bitcast are "same size" and
that pointers can only be casted to pointers. The restrictions on
isLosslesslyConvertibleTo are more stringent.
I'm going to rename it back to isLosslesslyConvertibleTo and retain its
use where it is needed. There are many cases, however, where a check for
bitcast is more appropriate and you've called many of them out.
>
>
> @@ -7107,11 +7250,11 @@ Instruction *InstCombiner::FoldPHIArgOpI
> PhiVal = NewPN;
> }
>
>
>
> // Insert and return the new operation.
> if (isa<CastInst>(FirstInst))
> - return new CastInst(PhiVal, PN.getType());
> + return CastInst::createInferredCast(PhiVal, PN.getType());
>
>
> This is a bug, use the opcode from FirstInst.
Four! Thank you.
>
> -Chris
More information about the llvm-commits
mailing list