On Dec 12, 2006, at 3:36 PM, Reid Spencer wrote:
> @@ -3286,7 +3281,8 @@
> Op1C-
> >getOperand(0),
> I.getName());
> InsertNewInstBefore(NewOp, I);
> - return CastInst::createInferredCast(NewOp, I.getType());
> + return CastInst::createIntegerCast(NewOp, I.getType(),
> + SrcTy->isSigned());
> }
> }
> }
> @@ -3690,7 +3686,8 @@
> Op1C-
> >getOperand(0),
> I.getName());
> InsertNewInstBefore(NewOp, I);
> - return CastInst::createInferredCast(NewOp, I.getType());
> + return CastInst::createIntegerCast(NewOp, I.getType(),
> + SrcTy->isSigned());
> }
> }
>
> @@ -3871,7 +3868,8 @@
> Op1C-
> >getOperand(0),
> I.getName());
> InsertNewInstBefore(NewOp, I);
> - return CastInst::createInferredCast(NewOp, I.getType());
> + return CastInst::createIntegerCast(NewOp, I.getType(),
> + SrcTy->isSigned());
> }
> }
These three all point out serious bugs. The code looks like this for
each:
// fold (and (cast A), (cast B)) -> (cast (and A, B))
if (CastInst *Op1C = dyn_cast<CastInst>(Op1)) {
if (CastInst *Op0C = dyn_cast<CastInst>(Op0)) {
const Type *SrcTy = Op0C->getOperand(0)->getType();
if (SrcTy == Op1C->getOperand(0)->getType() && SrcTy-
>isIntegral() &&
// Only do this if the casts both really cause code to be
generated.
ValueRequiresCast(Op0C->getOperand(0), I.getType(), TD) &&
ValueRequiresCast(Op1C->getOperand(0), I.getType(), TD)) {
Instruction *NewOp = BinaryOperator::createAnd(Op0C-
>getOperand(0),
Op1C-
>getOperand(0),
I.getName());
InsertNewInstBefore(NewOp, I);
return CastInst::createIntegerCast(NewOp, I.getType(),
SrcTy->isSigned());
}
}
}
This xform used to be safe before the cast patch, but now can turn
stuff like:
(and (sext A), (zext B))
into:
(sext (and A, B))
which is very wrong (again, never use SrcTy->isSigned!).
All three of these cases should verify that Op1C->getOpcode() == Op2C-
>getOpcode(), and it should use the opcode in the inserted cast.
That is, either of these are allowed:
(and (sext A), (sext B)) -> (sext (and A, B))
(and (zext A), (zext B)) -> (zext (and A, B))
but no mixing.
> @@ -5392,8 +5389,7 @@
>
> Value *Op = ShiftOp->getOperand(0);
> if (isShiftOfSignedShift != isSignedShift)
> - Op = InsertNewInstBefore(
> - CastInst::createInferredCast(Op, I.getType(),
> "tmp"), I);
> + Op = InsertNewInstBefore(new BitCastInst(Op, I.getType(),
> "tmp"), I);
This cast can be removed, now that shifts are signless.
> @@ -5681,7 +5677,7 @@
> /// evaluate the expression.
> Value *InstCombiner::EvaluateInDifferentType(Value *V, const Type
> *Ty) {
> if (Constant *C = dyn_cast<Constant>(V))
> - return ConstantExpr::getCast(C, Ty);
> + return ConstantExpr::getIntegerCast(C, Ty, C->getType()-
> >isSigned());
This looks extremely unsafe. Why is it ok?
> @@ -7950,13 +7943,20 @@
> // the same size. Instead of casting the pointer before
> the store, cast
> // the value to be stored.
> Value *NewCast;
> - if (Constant *C = dyn_cast<Constant>(SI.getOperand(0)))
> - NewCast = ConstantExpr::getCast(C, SrcPTy);
> + Instruction::CastOps opcode = Instruction::BitCast;
> + Value *SIOp0 = SI.getOperand(0);
> + if (SrcPTy->getTypeID() == Type::PointerTyID) {
isa<PointerType>(SrcPTy)
> + if (SIOp0->getType()->isIntegral())
> + opcode = Instruction::IntToPtr;
> + } else if (SrcPTy->isIntegral()) {
> + if (SIOp0->getType()->getTypeID() == Type::PointerTyID)
isa<PointerType>
-Chris
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
