[llvm-commits] CAST patch: #5: Instcombine
Chris Lattner
clattner at apple.com
Tue Nov 21 11:34:46 PST 2006
On Nov 18, 2006, at 11:24 AM, Reid Spencer wrote:
> Chris,
>
> Here's the CAST patch. This passes 100% of llvm/test and llvm-test.
> The
> order of the files in the patch has been set up for logical review.
> Some
> of this you've already seen, but its changed so much that those things
> should probably be reviewed again.
There are some serious bugs here. Please fix them and ask questions
about whatever doesn't make sense.
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.
Thanks Reid, I'm looking forward to this landing.
-Chris
+/// 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.
@@ -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.
IntToPtr shouldn't be handled here, this function is never called on
pointers.
+ 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.
+ 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.
+ // Must be a truncate so just look in the input
This comment is not needed any more :)
@@ -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.
@@ -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.
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.
@@ -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>
@@ -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>
@@ -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.
@@ -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.
@@ -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.
+/// @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.
// 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.
+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.
+ } 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.
+ case Instruction::PtrToInt:
+ case Instruction::IntToPtr:
These cases are dead, you know the src/dst are both integers.
+ 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.
+ 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.
+ 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
+ // 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.
+ 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
+ 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
+Instruction *InstCombiner::visitTrunc(CastInst &CI) {
+ return this->commonIntCastTransforms(CI);
+}
Why 'this->' ?
+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.
@@ -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.
@@ -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.
@@ -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.
@@ -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.
@@ -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).
@@ -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.
@@ -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).
@@ -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.
@@ -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.
-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061121/3588e705/attachment.html>
More information about the llvm-commits
mailing list