[llvm-commits] CAST patch (For Review Only) Part #1: include + vmcore part
Reid Spencer
rspencer at reidspencer.com
Mon Nov 20 17:09:33 PST 2006
Feedback ..
On Mon, 2006-11-20 at 13:27 -0800, Chris Lattner wrote:
>
> 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.
Done.
>
>
> 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.
I turned it into this:
@@ -744,21 +735,24 @@ static Constant *CastConstantPacked(Cons
// If both vectors have the same number of elements (thus, the
elements
// 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.
- if (SrcEltTy->isIntegral() && DstEltTy->isIntegral()) {
+ // If the src and dest elements are both integers, or both floats,
we can
+ // just BitCast each element because the elements are the same
size.
+ if ((SrcEltTy->isIntegral() && DstEltTy->isIntegral()) ||
+ (SrcEltTy->isFloatingPoint() && DstEltTy->isFloatingPoint())) {
for (unsigned i = 0; i != SrcNumElts; ++i)
- Result.push_back(ConstantExpr::getCast(CP->getOperand(i),
- DstEltTy));
+ Result.push_back(
+ ConstantExpr::getCast(Instruction::BitConvert,
CP->getOperand(1),
+ DstEltTy));
return ConstantPacked::get(Result);
}
+ // If this is an int-to-fp cast ..
if (SrcEltTy->isIntegral()) {
- // Otherwise, this is an int-to-fp cast.
+ // Ensure that it is int-to-fp cast
assert(DstEltTy->isFloatingPoint());
if (DstEltTy->getTypeID() == Type::DoubleTyID) {
for (unsigned i = 0; i != SrcNumElts; ++i) {
double V =
BitsToDouble(cast<ConstantInt>(CP->getOperand(i))->getZExtValue());
>
> 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:
how?
>
>
> + 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;
what can abort here? The only possible thing is
cast<ConstantIntegral>(V) and that is checked with the
isa<ConstantIntegral>(V).
>
>
>
> 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.
Right we discussed this on IRC and I made a change that passes the test
case we developed. It uses the Rules to cast to unsigned and then again
to cast to signed.
>
> // 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.
>
Done.
>
>
> +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).
What should it return in a release-asserts build? Returning 0 will make
it fail (segv) very close to the actual problem.
>
> +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");
I replaced it with:
assert(isa<PointerType>(SrcTy) == isa<PointerType>(DstTy) &&
"Bitcast cannot cast pointer to non-pointer and vice versa");
because they must either both be a pointer or neither be a 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.
Easy change so I made it.
>
>
> + 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. :(
Right. Missed that one.
>
>
>
>
> +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:
Okay.
>
>
> +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.
>
Done.
>
>
>
>
> +/// 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.
This increases code size (common code x 12), but okay. Common code is 5
function calls and 4 stores.
>
>
> + 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.
Yup, I did.
>
>
> + // 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.
Fixed.
>
>
> -Chris
>
>
>
>
More information about the llvm-commits
mailing list