[llvm-commits] CAST patch: Part #2: .ll/.bc readers + Analysis
Reid Spencer
rspencer at reidspencer.com
Mon Nov 20 19:57:37 PST 2006
On Mon, 2006-11-20 at 14:07 -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.
>
> The two occurrences of:
>
>
> + if ($1.obsolete && ($4->get() == Type::BoolTy)) {
> + // The previous definition of cast to bool was a compare
> against zero.
> + // We have to retain that semantic so we do it here.
> + $$ = new SetCondInst(Instruction::SetEQ, $2,
> + Constant::getNullValue($2->getType()));
> + $1.obsolete = false;
>
>
> are wrong, this should be SetNE X, nullvalue. You got this right once
> in the bc reader, but this one is wrong:
Yikes. Nice catch.
>
>
> @@ -1585,13 +1660,23 @@ inline unsigned BytecodeReader::upgradeC
> Opcode = Instruction::SetGT;
> break;
> case 26: // GetElementPtr
> Opcode = Instruction::GetElementPtr;
> break;
> - case 28: // Cast
> - Opcode = Instruction::Cast;
> + case 28: { // Cast
> + const Type *Ty = getType(TypeID);
> + if (Ty == Type::BoolTy) {
> + // The previous definition of cast to bool was a compare
> against zero.
> + // We have to retain that semantic so we do it here.
> + Opcode = Instruction::SetEQ;
> + return ConstantExpr::get(Instruction::SetEQ, ArgVec[0],
> +
> Constant::getNullValue(ArgVec[0]->getType()));
> + } else {
> + Opcode = CastInst::getCastOpcode(ArgVec[0], Ty);
> + }
> break;
> + }
> case 30: // Shl
> Opcode = Instruction::Shl;
> break;
> case 31: // Shr
> if (ArgVec[0]->getType()->isSigned())
>
>
>
>
> diff -t -d -u -p -5 -r1.38 Reader.h
> --- lib/Bytecode/Reader/Reader.h 14 Nov 2006 04:47:22 -0000 1.38
> +++ lib/Bytecode/Reader/Reader.h 18 Nov 2006 19:22:09 -0000
> @@ -300,10 +301,12 @@ private:
> // In version 7, the Shr, Cast and Setcc instructions changed to
> their
> // signed counterparts. This flag will be true if these
> instructions are
> // signless (version 6 and prior).
> bool hasSignlessShrCastSetcc;
>
>
>
> + bool hasSingleCast;
> +
>
>
> This needs a comment.
No it needs to be removed. Not implementing backwards compat until bc
version 8.
>
>
>
> @@ -1345,29 +1343,39 @@ SCEVHandle ScalarEvolutionsImpl::createN
> ///
> SCEVHandle ScalarEvolutionsImpl::createNodeForCast(CastInst *CI) {
> const Type *SrcTy = CI->getOperand(0)->getType();
> const Type *DestTy = CI->getType();
>
>
>
> - // If this is a noop cast (ie, conversion from int to uint), ignore
> it.
> - if (SrcTy->isLosslesslyConvertibleTo(DestTy))
> + // If this is a noop cast (eg, conversion from int to uint), ignore
> it.
> + // NOTE: We should be passing in TargetData->getIntPtrType() to
> isNoopCast
> + // here for more accurate Int->Ptr and Ptr->Int no-op detection.
> Using
> + // ULongTy pessimizes this check for 32-bit platforms.
> + if (CI->isNoopCast(Type::ULongTy))
> return getSCEV(CI->getOperand(0));
>
>
>
> I believe that this is subtly wrong. The SCEV code doesn't really
> handle pointers correctly, and the old isLosslesslyConvertibleTo
> method would not return true for int<->ptr casts. Please change this
> to check for isa<BitCast>, which is equivalent to the old code.
> Please also remove the NOTE comment.
Okay, fixed.
>
>
>
> + // The cast operations can deal with bool and other types here. We
> only want
> + // integer casts so check that condition.
> if (SrcTy->isInteger() && DestTy->isInteger()) {
> + switch (CI->getOpcode()) {
> + case Instruction::Trunc:
> + return SCEVTruncateExpr::get(getSCEV(CI->getOperand(0)),
> + DestTy->getUnsignedVersion());
> + case Instruction::ZExt:
> + return SCEVZeroExtendExpr::get(getSCEV(CI->getOperand(0)),
> + DestTy->getUnsignedVersion());
> + case Instruction::BitCast:
> + return SCEVZeroExtendExpr::get(getSCEV(CI->getOperand(0)),
> + DestTy->getUnsignedVersion());
> + break;
> + default:
> + // If this is a sign or zero extending cast and we can prove
> that the
> + // value will never overflow, we could do similar
> transformations. But,
> + // we can't handle this yet.
> + break;
> + }
>
>
> The Instruction::BitCast case has already been handled by the
> isa<BitCast> check above, plz remove it. Overall, I think it would be
> cleaner to split createNodeForCast into createNodeForBitCast,
> createNodeForZext and createNodeForTrunc than to try to handle these
> three cases in one method.
I sunk all of them into createNodeForCast's only user: createSCEV.
>
>
> +++ lib/Analysis/ValueNumbering.cpp 18 Nov 2006 19:22:11 -0000
> @@ -110,12 +110,16 @@ void BVNImpl::visitCastInst(CastInst &CI
> Function *F = I.getParent()->getParent();
>
>
>
> for (Value::use_iterator UI = Op->use_begin(), UE = Op->use_end();
> UI != UE; ++UI)
> if (CastInst *Other = dyn_cast<CastInst>(*UI))
> - // Check that the types are the same, since this code handles
> casts...
> - if (Other->getType() == I.getType() &&
> + // Check that the opcode is the same
> + if (Other->getOpcode() == Instruction::CastOps(I.getOpcode())
> &&
> + // Check that the destination types are the same
> + Other->getType() == I.getType() &&
> + // Check that the source types are the same
> + CI.getOperand(0)->getType() ==
> Other->getOperand(0)->getType() &&
> // Is it embedded in the same function? (This could be
> false if LHS
> // is a constant or global!)
> Other->getParent()->getParent() == F &&
> // Check to see if this new cast is not I.
> Other != &I) {
>
>
> There is no need to check this:
>
>
> + // Check that the source types are the same
> + CI.getOperand(0)->getType() ==
> Other->getOperand(0)->getType() &&
>
>
> Since you know that CI.getOperand(0) == Other->getOperand(0) already.
How do you know this? Other is a User of CI so how can you be certain
that their operands equivalent?
>
>
>
>
>
>
> --- lib/Analysis/IPA/GlobalsModRef.cpp 1 Oct 2006 22:46:33 -0000 1.23
> +++ lib/Analysis/IPA/GlobalsModRef.cpp 18 Nov 2006 19:22:12 -0000
> @@ -165,14 +165,17 @@ static Value *getUnderlyingObject(Value
> // If we are at some type of object... return it.
> if (GlobalValue *GV = dyn_cast<GlobalValue>(V)) return GV;
>
>
>
> // Traverse through different addressing mechanisms.
> if (Instruction *I = dyn_cast<Instruction>(V)) {
> - if (isa<CastInst>(I) || isa<GetElementPtrInst>(I))
> + if (isa<BitCastInst>(I) ||
> + isa<IntToPtrInst>(I) ||
> + isa<GetElementPtrInst>(I))
> return getUnderlyingObject(I->getOperand(0));
> } else if (ConstantExpr *CE = dyn_cast<ConstantExpr>(V)) {
> - if (CE->getOpcode() == Instruction::Cast ||
> + if (CE->getOpcode() == Instruction::BitCast ||
> + CE->getOpcode() == Instruction::IntToPtr ||
> CE->getOpcode() == Instruction::GetElementPtr)
> return getUnderlyingObject(CE->getOperand(0));
> }
>
>
>
> // Othewise, we don't know what this is, return it as the base
> pointer.
>
>
>
>
> You don't want to include IntToPtr in either of these.
>
>
>
>
> @@ -250,12 +253,11 @@ bool GlobalsModRef::AnalyzeUsesOfPointer
> // Make sure that this is just the function being called, not
> that it is
> // passing into the function.
> for (unsigned i = 3, e = II->getNumOperands(); i != e; ++i)
> if (II->getOperand(i) == V) return true;
> } else if (ConstantExpr *CE = dyn_cast<ConstantExpr>(*UI)) {
> - if (CE->getOpcode() == Instruction::GetElementPtr ||
> - CE->getOpcode() == Instruction::Cast) {
> + if (CE->getOpcode() == Instruction::GetElementPtr ||
> CE->isCast()) {
> if (AnalyzeUsesOfPointer(CE, Readers, Writers))
> return true;
>
>
> This should just allow GEP and BitCast, not any Cast.
Okay.
>
>
> -Chris
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
More information about the llvm-commits
mailing list