[llvm-commits] CAST patch (For Review Only) Part #1: include + vmcore part
Chris Lattner
clattner at apple.com
Mon Nov 20 13:27:33 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.
>
> Thanks,
>
> Reid.
> <CAST.patch>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
diff -t -d -u -p -5 -r1.212 AsmWriter.cpp
--- lib/VMCore/AsmWriter.cpp 2 Nov 2006 20:25:50 -0000 1.212
+++ lib/VMCore/AsmWriter.cpp 18 Nov 2006 19:21:59 -0000
@@ -539,13 +539,28 @@ static void WriteConstantInt(std::ostrea
- if (CE->getOpcode() == Instruction::Cast) {
- Out << " to ";
- printTypeInt(Out, CE->getType(), TypeTable);
+ switch (CE->getOpcode()) {
+ default:
+ break;
+ case Instruction::Trunc:
+ case Instruction::ZExt:
+ case Instruction::SExt:
+ case Instruction::FPTrunc:
+ case Instruction::FPExt:
+ case Instruction::UIToFP:
+ case Instruction::SIToFP:
+ case Instruction::FPToUI:
+ case Instruction::FPToSI:
+ case Instruction::PtrToInt:
+ case Instruction::IntToPtr:
+ case Instruction::BitCast:
+ Out << " to ";
+ printTypeInt(Out, CE->getType(), TypeTable);
+ break;
}
Out << ')';
Plz use CE->isCast() intead of a switch stmt.
diff -t -d -u -p -5 -r1.99 ConstantFolding.cpp
--- lib/VMCore/ConstantFolding.cpp 8 Nov 2006 19:16:44 -0000 1.99
+++ lib/VMCore/ConstantFolding.cpp 18 Nov 2006 19:22:00 -0000
@@ -745,15 +736,26 @@ static Constant *CastConstantPacked(Cons
// are the same size), perform the conversion now.
if (SrcNumElts == DstNumElts) {
std::vector<Constant*> Result;
// If the src and dest elements are both integers, just cast
each one
- // which will do the appropriate bit-convert.
+ // which will do the appropriate conversion.
if (SrcEltTy->isIntegral() && DstEltTy->isIntegral()) {
+ Instruction::CastOps opcode = Instruction::Trunc;
+ unsigned SrcSize = SrcEltTy->getPrimitiveSizeInBits();
+ unsigned DstSize = DstEltTy->getPrimitiveSizeInBits();
+ if (SrcSize < DstSize) {
+ if (SrcEltTy->isUnsigned())
+ opcode = Instruction::ZExt;
+ else
+ opcode = Instruction::SExt;
+ } else if (SrcSize == DstSize) {
+ opcode = Instruction::BitCast;
+ }
for (unsigned i = 0; i != SrcNumElts; ++i)
- Result.push_back(ConstantExpr::getCast(CP->getOperand(i),
- DstEltTy));
+ Result.push_back(
+ ConstantExpr::getCast(opcode, CP->getOperand(i), DstEltTy));
return ConstantPacked::get(Result);
}
if (SrcEltTy->isIntegral()) {
// Otherwise, this is an int-to-fp cast.
This should not look at the signedness of the element type of the
packed type to determine which cast it does.
This should always be BitCast, never extend or truncate.
This code:
+ // We actually have to do a cast now, but first, we might need to
fix up
+ // the value of the operand.
+ switch (opc) {
appears to abort on constant expr operands in places like this:
+ case Instruction::IntToPtr: //always treated as unsigned
+ case Instruction::UIToFP:
+ case Instruction::ZExt:
+ // A ZExt always produces an unsigned value so we need to cast
the value
+ // now before we try to cast it to the destination type
+ if (isa<ConstantIntegral>(V))
+ V = ConstantInt::get(SrcTy->getUnsignedVersion(),
+ cast<ConstantIntegral>(V)->getZExtValue());
+ break;
ConstantFoldCastInstruction will miscompile a fptoui cast if the
destination is signed and fptosi if dest is unsigned. For example,
"sbyte fptouint float 255.0" should be defined and return -1 always.
// Ok, we have two differing integer indices. Sign extend them
to be the same
// type. Long is always big enough, so we use it.
- C1 = ConstantExpr::getSignExtend(C1, Type::LongTy);
- C2 = ConstantExpr::getSignExtend(C2, Type::LongTy);
+ if (C1->getType() != Type::LongTy && C1->getType() != Type::ULongTy)
+ C1 = ConstantExpr::getSignExtend(C1, Type::LongTy);
+ if (C2->getType() != Type::LongTy && C1->getType() != Type::ULongTy)
+ C2 = ConstantExpr::getSignExtend(C2, Type::LongTy);
Please change this to use bitconvert to turn ulong into longty. We
always want the operands to be LongTy going out of here.
+Constant *ConstantExpr::getCast(unsigned oc, Constant *C, const Type
*Ty) {
+ Instruction::CastOps opc = Instruction::CastOps(oc);
+ assert(Instruction::isCast(opc) && "opcode out of range");
+ assert(C && Ty && "Null arguments to getCast");
+ assert(Ty->isFirstClassType() && "Cannot cast to an aggregate
type!");
+
+ switch (opc) {
+ default:
+ break;
This should abort on an invalid cast opcode, not return null (the
default should not be 'break' then ret null).
+Constant *ConstantExpr::getTrunc(Constant *C, const Type *Ty) {
+ assert(C->getType()->isInteger() && "Trunc operand must be integer");
+ assert(Ty->isIntegral() && "Trunc produces only integral");
+ assert(C->getType()->getPrimitiveSizeInBits() > Ty-
>getPrimitiveSizeInBits()&&
+ "SrcTy must be larger than DestTy for Trunc!");
+
+ // Casts to boolean are really common, handle it here
+ if (Ty == Type::BoolTy) {
+ if (C->isNullValue())
+ return ConstantBool::getFalse();
+ else
+ return ConstantBool::getTrue();
+ }
+ return getFoldedCast(Instruction::Trunc, C, Ty);
+}
The "common case" here is both uncommon and incorrect (for
constantexprs) plz remove it.
Constant *ConstantExpr::getSignExtend(Constant *C, const Type *Ty) {
+ assert(C->getType()->isIntegral() && "SEXt operand must be
integral");
+ assert(Ty->isInteger() && "SExt produces only integer");
+ assert(C->getType()->getPrimitiveSizeInBits() < Ty-
>getPrimitiveSizeInBits()&&
+ "SrcTy must be smaller than DestTy for SExt!");
+
+ // Casts to/from boolean are really common, handle it here
+ if (C->getType() == Type::BoolTy) {
if (C == ConstantBool::getTrue())
return ConstantIntegral::getAllOnesValue(Ty);
else
return ConstantIntegral::getNullValue(Ty);
}
+ return getFoldedCast(Instruction::SExt, C, Ty);
}
Likewise, the "Casts to/from boolean are really common, handle it
here" code is also incorrect, please remove, likewise in getZeroExtend.
+Constant *ConstantExpr::getBitCast(Constant *C, const Type *DstTy) {
+ // BitCast implies a no-op cast of type only. No bits change.
However, you
+ // can't cast pointers to anything but pointers.
+ const Type *SrcTy = C->getType();
+ if (isa<PointerType>(SrcTy)) {
+ assert(isa<PointerType>(DstTy) &&
+ "Bitcast can not cast pointer to non-pointer");
+ } else if (isa<PointerType>(DstTy)) {
+ assert(isa<PointerType>(SrcTy) &&
+ "Bitcast can not cast non-pointer to pointer");
+ }
The else should be:
+ } else {
+ assert(!isa<PointerType>(DstTy) &&
+ "Bitcast can not cast non-pointer to pointer");
+ }
Better yet, write this if/then like this:
assert(isa<PointerType>(SrcTy) == isa<PointerType>(DstTy) &&
"Bitcast can not cast non-pointer to pointer");
+ // Now we know we're not dealing with pointers. For all the other
types, the
This comment isn't correct, pointers can get here.
+ // cast is okay if source and destination bit widths are
identical. Be
+ // careful to correctly get the width of packed types here.
+ unsigned SrcBitSize = SrcTy->getPrimitiveSizeInBits();
+ unsigned DstBitSize = DstTy->getPrimitiveSizeInBits();
+ if (const PackedType *SrcPTy = dyn_cast<PackedType>(SrcTy))
+ SrcBitSize = SrcPTy->getBitWidth();
+ if (const PackedType *DstPTy = dyn_cast<PackedType>(DstTy))
+ DstBitSize = DstPTy->getBitWidth();
We should make getPrimitiveSizeInBits return the right thing for
packed types, but this can be a later patch.
+ assert(SrcBitSize == DstBitSize && "Bitcast requies types of same
width");
+ return getFoldedCast(Instruction::BitCast, C, DstTy);
@@ -1856,11 +2030,11 @@ void ConstantExpr::replaceUsesOfWithOnCo
Constant *Val = getOperand(i);
if (Val == From) Val = To;
Indices.push_back(Val);
}
Replacement = ConstantExpr::getGetElementPtr(Pointer, Indices);
- } else if (getOpcode() == Instruction::Cast) {
+ } else if (isCast()) {
assert(getOperand(0) == From && "Cast only has one use!");
Replacement = ConstantExpr::getCast(To, getType());
} else if (getOpcode() == Instruction::Select) {
Constant *C1 = getOperand(0);
Constant *C2 = getOperand(1);
This is certainly a bug, you need to pass the opcode in here. This
is an example of how getCast is totally unsafe. :(
+bool CastInst::isNoopCast(const Type *IntPtrTy) const {
+ switch (getOpcode()) {
+ case Instruction::Trunc:
+ case Instruction::ZExt:
+ case Instruction::SExt:
+ case Instruction::FPTrunc:
+ case Instruction::FPExt:
+ case Instruction::UIToFP:
+ case Instruction::SIToFP:
+ case Instruction::FPToUI:
+ case Instruction::FPToSI:
+ return false; // These always modify bits
+ case Instruction::BitCast:
+ return true; // BitCast never modifies bits.
+ case Instruction::PtrToInt:
+ return IntPtrTy->getPrimitiveSizeInBits() ==
+ getType()->getPrimitiveSizeInBits();
+ case Instruction::IntToPtr:
+ return IntPtrTy->getPrimitiveSizeInBits() ==
+ getOperand(0)->getType()->getPrimitiveSizeInBits();
+ default:
+ assert(!"Invalid CastOp");
+ return false;
+ }
+}
This will cause code to be generated in release builds, please handle
default like this:
+bool CastInst::isNoopCast(const Type *IntPtrTy) const {
+ switch (getOpcode()) {
+ default: assert(0 && "Invalid CastOp");
+ case Instruction::Trunc:
+ case Instruction::ZExt:
+ case Instruction::SExt:
+ case Instruction::FPTrunc:
+ case Instruction::FPExt:
+ case Instruction::UIToFP:
+ case Instruction::SIToFP:
+ case Instruction::FPToUI:
+ case Instruction::FPToSI:
+ return false; // These always modify bits
+ case Instruction::BitCast:
+ return true; // BitCast never modifies bits.
+ case Instruction::PtrToInt:
+ return IntPtrTy->getPrimitiveSizeInBits() ==
+ getType()->getPrimitiveSizeInBits();
+ case Instruction::IntToPtr:
+ return IntPtrTy->getPrimitiveSizeInBits() ==
+ getOperand(0)->getType()->getPrimitiveSizeInBits();
+ }
+}
+Instruction::CastOps
+CastInst::getCastOpcode(const Value *Src, const Type *DestTy) {
..
+ if (DestBits != PTy->getBitWidth()) {
+ assert(!"Casting packed to integer of different width");
+ } else {
+ return BitCast; // Same size, no-
op cast
+ }
This causes code to be generated in non-assert builds. Use this
instead:
assert(DestBits == PTy->getBitWidth() && "Casting packed to integer
of different width");
return BitCast; // Same size, no-op cast
Likewise here:
+ } else if (isa<PointerType>(SrcTy)) {
+ return PtrToInt; // ptr -> int
+ } else {
+ assert(!"Casting from a value that is not first-class type");
+ }
and many other examples in this method.
+ } else if (isa<PointerType>(DestTy)) {
+ if (isa<PointerType>(SrcTy)) {
+ return BitCast; // ptr -> ptr
+ } else if (SrcTy->isIntegral()) {
+ return IntToPtr; // int -> ptr
+ } else {
+ assert(!"Casting pointer to other than pointer or int");
+ }
This doesn't handle 'cast float 0.0 to int*', nor can it. I maintain
that this method is fundamentally flawed, please make sure no callers
can pass this in. Please verify Transforms/ConstProp/float-to-ptr-
cast.ll passes.
+TruncInst::TruncInst(
+ Value *S, const Type *Ty, const std::string &Name, Instruction
*InsertBefore
+) : CastInst(Ty, Trunc, S, Name, InsertBefore) {
+ checkCast(getOpcode(), S, Ty);
+}
(and others). Please change checkCast to return true always, then
change callers to be:
+TruncInst::TruncInst(
+ Value *S, const Type *Ty, const std::string &Name, Instruction
*InsertBefore
+) : CastInst(Ty, Trunc, S, Name, InsertBefore) {
+ assert(checkCast(getOpcode(), S, Ty));
+}
This makes it more clear that no code will be generated in assert-
disabled builds.
+/// visitCastInst - Provide some basic sanity checks on the various
+/// CastInst subclasses to ensure their operands are right for the
kind of cast
+/// requested.
+void Verifier::visitCastInst(CastInst &I) {
..
+ // Basic sanity .. types must be first class
+ Assert1(SrcTy->isFirstClassType(),"Can only cast to first-class
type", &I);
+ Assert1(DestTy->isFirstClassType(), "Can only cast from first-
class type",&I);
+
These two checks are done by visitInstruction already.
+ // Check each kind of CastInst separately
+ switch (I.getOpcode()) {
+ case Instruction::Trunc:
+ Assert1(SrcTy->isIntegral(), "Trunc only operates on integer", &I);
+ Assert1(DestTy->isIntegral(),"Trunc only produces integral", &I);
+ Assert1(SrcBitSize > DestBitSize,"DestTy too big for Trunc", &I);
+ break;
Please use the instvisitor switch to do this, there is no reason to
lump these all together into visitCastInst and then switch on the
opcode again.
+ case Instruction::BitCast: {
+ // BitCast implies a no-op cast of type only. No bits change.
+ // However, you can't cast pointers to anything but pointers.
+ if (isa<PointerType>(SrcTy)) {
+ Assert1(isa<PointerType>(DestTy),
+ "Bitcast can not cast pointer to non-pointer", &I);
+ break;
+ } else if (isa<PointerType>(DestTy)) {
+ Assert1(isa<PointerType>(SrcTy),
+ "Bitcast can not cast non-pointer to pointer", &I);
+ break;
+ }
Use the same isa<ptr> == isa<ptr> trick from before.
+ // Now we know we're not dealing with pointers. For all the
other types, the
+ // cast is okay if source and destination bit widths are identical.
+ // Be careful to get the width of packed types correctly here.
comment wrong again.
-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061120/b66c6f29/attachment.html>
More information about the llvm-commits
mailing list