[llvm-commits] CAST Patch Update [1/4] first third of the patch.
Chris Lattner
clattner at apple.com
Sun Nov 12 17:51:46 PST 2006
> Attached is the latest CAST patch. This passes all llvm/test and
> llvm-test test cases with the exception of MultiSource/Applications/
> hbd.
> I'm hoping that fresh eyes on this patch can help discover the problem
> because it is a mystery I have not solved in two weeks.
I will look, but you're *not* approved to commit this until hbd works.
> The problem is not a mis-optimization. The unoptimized bytecode fails
> the test as does every permutation of the gccas optimization passes.
> Except for the fact that the nightly test passes this test, I'm
> unconvinced that this is directly related to the CAST patch. Its
> possible that the CAST patch exposes a bug somewhere else.
Perhaps the front-end?
> This difference output is one reason why I *do* think the problem
> could
> be related to the CAST patch or at least some of the signless types
> work. It appears to be doing a sign extension instead of a zero
> extension on a boolean to arrive at the decrement value.
It's clearly a bug, and I'll obviously be looking for bugs, but the
responsibility is ultimately yours to figure it out. :)
----
Meta issues:
Please send the llvm-gcc patch as well.
I realize Sheng is probably working on it, but plz make sure the
ConstantExpr::getUShr patch happens.
I really think we should drop all bc stuff prior to version 3 (LLVM
1.3) or 5 (1.4) asap. I think this will eliminate some amount of
complexity from the bcreader. 1.4 is nearly 2 years old now :).
---
On to the patch:
+ // @brief Get a ConstantExpr Conversion operator that casts C to Ty
+ static Constant* getCast( Constant *C, const Type* Ty );
Note that this will have to be removed when signless finally lands,
but I'm fine with it existing until then. Please remove the spaces
from around the arguments.
--- include/llvm/DerivedTypes.h 7 Feb 2006 06:17:10 -0000 1.71
+++ include/llvm/DerivedTypes.h 12 Nov 2006 06:51:26 -0000
Please commit this independently of your page (e.g. asap), it looks
fine.
--- include/llvm/InstrTypes.h 17 Sep 2006 19:29:56 -0000 1.47
+++ include/llvm/InstrTypes.h 12 Nov 2006 06:51:26 -0000
@@ -15,10 +15,11 @@
#ifndef LLVM_INSTRUCTION_TYPES_H
#define LLVM_INSTRUCTION_TYPES_H
#include "llvm/Instruction.h"
+#include "llvm/Type.h"
Please remove this #include, by moving stuff out-of-line as appropriate.
Change "(if its non-null)" -> "(if it is non-null)", multiple instances.
+ static ConvertInst* get(
+ Instruction::ConvertOps, ///< The opcode of the data conversion
instruction
+ Value *S, ///< The value to be converted (operand 0)
+ const Type *Ty, ///< The type to which conversion
should be made
+ const std::string &Name = "", ///< Name for the instruction
+ Instruction *InsertBefore = 0 ///< Place to insert the ConvertInst
...
+ static ConvertInst* get(
+ Instruction::ConvertOps, ///< The opcode for the data conversion
instruction
+ Value* S, ///< The value to be converted (operand 0)
+ const Type *Ty, ///< The type to which conversion
should be made
+ const std::string &Name, ///< The name for the instruction
+ BasicBlock* InsertAtEnd ///< The block to insert the
instruction into
Your code is extremely inconsistent about * placement. Since the
rest of the LLVM code puts the space before the * (and half of your
code does to), I'd appreciate it if you were consistent and put the
space before the *.
+ /// Returns the opcode necessary to convert Val into Ty using
usual casting
+ /// rules.
+ static Instruction::ConvertOps getCastOpcode(
+ const Value* Val, ///< The value to cast
+ const Type* Ty ///< The Type to which the value should be casted
+ );
Like ConstantExpr::getCast (and ConvertInst::getCast), this will have
to go away eventually, but I'm fine with it in the meantime. As a
meta-issue, you call these things "convert"s in some places (e.g.
ConvertInst) and "cast"s in others. Please standardize.
+ /// For example, a cast from long to uint or uint to short.
+ /// @returns true if this is a truncating integer cast instruction
+ /// @brief Determine if this is a truncating integer cast
+ bool isTruncIntCast() const;
Why doesn't isa<truncinstruction> work for this?
+ /// A no-op cast is one that can be effected without changing any
bits.
+ /// It implies that the source and destination types are the same
size.
+ /// @brief Determine if this cast is a no-op cast.
+ bool isNoopCast(const Type* IntPtrTy = Type::ULongTy) const;
Why not isa<bitcastinst>? What is IntPtrTy? This isn't safe.
+ /// @brief Abstract clone() method (delegated to its subclasses).
+ virtual ConvertInst *clone() const = 0;
+
Please just remove this.
--- include/llvm/Instruction.def 11 Nov 2006 23:06:47 -0000 1.24
+++ include/llvm/Instruction.def 12 Nov 2006 06:51:26 -0000
...
+// NOTE: The order matters here, see InstCombine
(isEliminableCastOfCast)
This method is now in vmcore, not instcombine, right? In addition,
it got renamed.
--- include/llvm/Instruction.h 30 Sep 2006 22:20:34 -0000 1.74
+++ include/llvm/Instruction.h 12 Nov 2006 06:51:26 -0000
+ /// @brief Determine if the OpCode is one of the ConvertInst
instructions.
+ static inline bool isConvert(unsigned OpCode) {
+ return OpCode >= ConvertOpsBegin && OpCode < ConvertOpsEnd;
+ }
+
+ /// @brief Determine if this is one of the ConvertInst instructions.
+ inline bool isConvert() const {
+ return isConvert(getOpcode());
+ }
Why not isa<ConvertInst> ?
===================================================================
RCS file: /var/cvs/llvm/llvm/include/llvm/Instructions.h,v
retrieving revision 1.45
diff -t -d -u -p -5 -r1.45 Instructions.h
--- include/llvm/Instructions.h 8 Nov 2006 06:47:32 -0000 1.45
+++ include/llvm/Instructions.h 12 Nov 2006 06:51:27 -0000
@@ -475,48 +475,10 @@ public:
+/// @brief This class represents a cast from floating point to
signed integer.
+class FPToSIInst : public ConvertInst {
...
+ static inline bool classof(const Instruction *I) {
+ return I->getOpcode() == FPToUI;
+ }
That is a serious bug.
--- include/llvm/Support/InstVisitor.h 9 Oct 2006 18:33:08 -0000 1.41
+++ include/llvm/Support/InstVisitor.h 12 Nov 2006 06:51:27 -0000
@@ -141,11 +141,10 @@ public:
InstVisitor should allow visiting each ConvertInst subclass, and each
should default delegate to ConvertInst.
--- include/llvm/Support/PatternMatch.h 8 Nov 2006 06:47:32 -0000 1.13
+++ include/llvm/Support/PatternMatch.h 12 Nov 2006 06:51:27 -0000
cast_match will be useless when signlessness happens, and any uses
are almost certainly a bug right now (a zext from a signed to
unsigned value would be treated as a sext). Please remove it either
as part of this patch or as a follow-on. It may be dead with your
patch.
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/Analysis/BasicAliasAnalysis.cpp,v
retrieving revision 1.89
diff -t -d -u -p -5 -r1.89 BasicAliasAnalysis.cpp
if (const Instruction *I = dyn_cast<Instruction>(V)) {
- if (isa<CastInst>(I) || isa<GetElementPtrInst>(I))
+ if (isa<ConvertInst>(I) || isa<GetElementPtrInst>(I))
return getUnderlyingObject(I->getOperand(0));
} else if (const ConstantExpr *CE = dyn_cast<ConstantExpr>(V)) {
- if (CE->getOpcode() == Instruction::Cast ||
- CE->getOpcode() == Instruction::GetElementPtr)
+ if (CE->isConvert() || CE->getOpcode() ==
Instruction::GetElementPtr)
return getUnderlyingObject(CE->getOperand(0));
These should both only allow bitconvert, not any conversion.
} else if (const GlobalValue *GV = dyn_cast<GlobalValue>(V)) {
+ // FIXME: Isn't this redundant with hasUniqueAddress check above?
return GV;
}
Yes, I removed it.
@@ -203,17 +203,24 @@ static bool AddressMightEscape(const Val
+ // If this is a pointer -> pointer conversion ...
+ if (isa<PointerType>(I->getType()) &&
+ isa<PointerType>(I->getOperand(0)->getType()))
+ // Recurse on this use and return true only if that use
might escape
+ return AddressMightEscape(I);
+ // The use wasn't a pointer -> pointer conversion, so it escapes
+ return true;
This function is only invoked on pointers, meaning that this code can
all be reduced to:
return AddressMightEscape(I);
@@ -257,30 +264,28 @@ BasicAliasAnalysis::getModRefInfo(CallSi
AliasAnalysis::AliasResult
BasicAliasAnalysis::alias(const Value *V1, unsigned V1Size,
const Value *V2, unsigned V2Size) {
// Strip off any constant expression casts if they exist
if (const ConstantExpr *CE = dyn_cast<ConstantExpr>(V1))
- if (CE->getOpcode() == Instruction::Cast &&
- isa<PointerType>(CE->getOperand(0)->getType()))
+ if (CE->isConvert() && isa<PointerType>(CE->getOperand(0)-
>getType()))
V1 = CE->getOperand(0);
if (const ConstantExpr *CE = dyn_cast<ConstantExpr>(V2))
- if (CE->getOpcode() == Instruction::Cast &&
- isa<PointerType>(CE->getOperand(0)->getType()))
+ if (CE->isConvert() && isa<PointerType>(CE->getOperand(0)-
>getType()))
V2 = CE->getOperand(0);
Only bitcast, not general conversions.
// Strip off cast instructions...
- if (const Instruction *I = dyn_cast<CastInst>(V1))
+ if (const Instruction *I = dyn_cast<ConvertInst>(V1))
if (isa<PointerType>(I->getOperand(0)->getType()))
return alias(I->getOperand(0), V1Size, V2, V2Size);
- if (const Instruction *I = dyn_cast<CastInst>(V2))
+ if (const Instruction *I = dyn_cast<ConvertInst>(V2))
if (isa<PointerType>(I->getOperand(0)->getType()))
return alias(V1, V1Size, I->getOperand(0), V2Size);
If you restrict this to bitcasts, not general converts, you can get
rid of the "if (isa<" checks.
if (Constant *C1 = dyn_cast<Constant>(V1))
if (Constant *C2 = dyn_cast<Constant>(V2)) {
- // Sign extend the constants to long types.
- C1 = ConstantExpr::getSignExtend(C1, Type::LongTy);
- C2 = ConstantExpr::getSignExtend(C2, Type::LongTy);
+ // Sign extend the constants to long types, if necessary
+ if (C1->getType()->getPrimitiveSizeInBits() <
+ Type::LongTy->getPrimitiveSizeInBits())
+ C1 = ConstantExpr::getSignExtend(C1, Type::LongTy);
+ if (C2->getType()->getPrimitiveSizeInBits() <
+ Type::LongTy->getPrimitiveSizeInBits())
+ C2 = ConstantExpr::getSignExtend(C2, Type::LongTy);
Long is always 64-bits. Please "Constant fold" 'Type::LongTy-
>getPrimitiveSizeInBits()' here and elsewhere.
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/Analysis/ScalarEvolution.cpp,v
retrieving revision 1.58
diff -t -d -u -p -5 -r1.58 ScalarEvolution.cpp
+ if (CI->isNoopCast())
+ return getSCEV(CI->getOperand(0));
This should use isa<BitCast> and check that the input has integral type.
+ switch (CI->getOpcode()) {
+ case Instruction::Trunc:
+ return SCEVTruncateExpr::get(getSCEV(CI->getOperand(0)),
DestTy);
+ case Instruction::ZExt:
+ case Instruction::PtrToInt:
+ case Instruction::IntToPtr:
+ case Instruction::BitCast:
+ return SCEVZeroExtendExpr::get(getSCEV(CI->getOperand(0)),
DestTy);
+ break;
+ default:
+ // If this is an 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;
+ }
SCEV doesn't handle pointers, and it bitcast should be handled
above. Please remove all cases except Trunc/ZExt. Likewise in this
hunk:
@@ -1399,16 +1400,27 @@ SCEVHandle ScalarEvolutionsImpl::createS
--- lib/Analysis/ScalarEvolutionExpander.cpp 20 Oct 2006 07:07:24
-0000 1.4
+++ lib/Analysis/ScalarEvolutionExpander.cpp 12 Nov 2006 06:51:29 -0000
@@ -27,31 +27,30 @@ Value *SCEVExpander::InsertCastOfTo(Valu
- return new CastInst(V, Ty, V->getName(),
+ return ConvertInst::getCast(V, Ty, V->getName(),
A->getParent()->getEntryBlock().begin());
This is *extremely* dangerous. Have you proven that this is safe?
Because you are disassociating the type from the operation, this
seems like it could insert incorrect casts (the sign of the input
type [which ConvertInst::getCast uses] need not have anything to do
with the cast operation). I think you should totally eliminate
ConvertInst::getCast and the ConstantExpr version as well.
@@ -63,11 +62,11 @@ Value *SCEVExpander::InsertCastOfTo(Valu
}
BasicBlock::iterator IP = I; ++IP;
if (InvokeInst *II = dyn_cast<InvokeInst>(I))
IP = II->getNormalDest()->begin();
while (isa<PHINode>(IP)) ++IP;
- return new CastInst(V, Ty, V->getName(), IP);
+ return ConvertInst::getCast(V, Ty, V->getName(), IP);
}
Likewise.
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/Analysis/ValueNumbering.cpp,v
retrieving revision 1.21
diff -t -d -u -p -5 -r1.21 ValueNumbering.cpp
--- lib/Analysis/ValueNumbering.cpp 28 Aug 2006 00:42:29 -0000 1.21
+++ lib/Analysis/ValueNumbering.cpp 12 Nov 2006 06:51:29 -0000
@@ -102,20 +102,24 @@ void BasicVN::getEqualNumberNodes(Value
+void BVNImpl::visitConvertInst(ConvertInst &CI) {
Instruction &I = (Instruction&)CI;
Value *Op = I.getOperand(0);
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() &&
+ if (ConvertInst *Other = dyn_cast<ConvertInst>(*UI))
+ // Check that the opcode is the same
+ if (Other->getOpcode() == Instruction::ConvertOps(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) == Other->getOperand(0) &&
This last line should check the *type* not the operand. This has the
effect of neutering value numbering of casts :)
diff -t -d -u -p -5 -r1.35 Andersens.cpp
--- lib/Analysis/IPA/Andersens.cpp 8 Nov 2006 06:47:33 -0000 1.35
+++ lib/Analysis/IPA/Andersens.cpp 12 Nov 2006 06:51:30 -0000
@@ -527,11 +527,23 @@ Andersens::Node *Andersens::getNodeForCo
return getNode(GV);
else if (ConstantExpr *CE = dyn_cast<ConstantExpr>(C)) {
switch (CE->getOpcode()) {
case Instruction::GetElementPtr:
return getNodeForConstantPointer(CE->getOperand(0));
- case Instruction::Cast:
+ 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::IntToPtr:
+ return &GraphNodes[UniversalSet];
+ case Instruction::PtrToInt:
+ case Instruction::BitCast:
if (isa<PointerType>(CE->getOperand(0)->getType()))
return getNodeForConstantPointer(CE->getOperand(0));
else
return &GraphNodes[UniversalSet];
default:
Most of these casts can't have results that are pointers. Replace
this with:
+ case Instruction::IntToPtr:
+ return &GraphNodes[UniversalSet];
+ case Instruction::BitCast:
+ return getNodeForConstantPointer(CE->getOperand(0));
Likewise in the second occurrence.
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/Analysis/IPA/GlobalsModRef.cpp,v
retrieving revision 1.23
diff -t -d -u -p -5 -r1.23 GlobalsModRef.cpp
--- lib/Analysis/IPA/GlobalsModRef.cpp 1 Oct 2006 22:46:33 -0000 1.23
+++ lib/Analysis/IPA/GlobalsModRef.cpp 12 Nov 2006 06:51:30 -0000
@@ -165,15 +165,14 @@ 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<ConvertInst>(I) || isa<GetElementPtrInst>(I))
return getUnderlyingObject(I->getOperand(0));
} else if (ConstantExpr *CE = dyn_cast<ConstantExpr>(V)) {
- if (CE->getOpcode() == Instruction::Cast ||
- CE->getOpcode() == Instruction::GetElementPtr)
+ if (CE->isConvert() || CE->getOpcode() ==
Instruction::GetElementPtr)
return getUnderlyingObject(CE->getOperand(0));
}
The two changes in this file both need to be made aware that the
target type can only be a pointer. This simplifies it. There is no
need to handle ConvertInst.
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/Bytecode/Reader/Reader.cpp,v
retrieving revision 1.204
diff -t -d -u -p -5 -r1.204 Reader.cpp
Your .ll/.bc changes don't autoupgrade the 'cast to bool -> setcc'
semantic. In particular trunc x to bool and fptouint bool do *not*
produce a comparison against zero.
--- lib/CodeGen/IntrinsicLowering.cpp 8 Nov 2006 06:47:33 -0000 1.45
+++ lib/CodeGen/IntrinsicLowering.cpp 12 Nov 2006 06:51:38 -0000
@@ -61,11 +61,12 @@ static CallInst *ReplaceCallWith(const c
The uses of ConvertInst::getCast are not safe here. Please make
callers of ReplaceCallWith pass in the convert opc to use.
Index: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/CodeGen/SelectionDAG/
SelectionDAGISel.cpp,v
retrieving revision 1.317
diff -t -d -u -p -5 -r1.317 SelectionDAGISel.cpp
--- lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp 10 Nov 2006
04:41:34 -0000 1.317
+++ lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp 12 Nov 2006
06:51:40 -0000
@@ -554,17 +554,29 @@ public:
+void SelectionDAGLowering::visitTrunc(User &I) {
+ // TruncInst cannot be a no-op cast because sizeof(src) > sizeof
(dest).
+ SDOperand N = getValue(I.getOperand(0));
+ MVT::ValueType DestVT = TLI.getValueType(I.getType());
+ if (DestVT == MVT::i1) {
+ // Cast to bool is a comparison against zero, not truncation to
zero.
+ SDOperand Zero = DAG.getConstant(0, N.getValueType());
+ setValue(&I, DAG.getSetCC(MVT::i1, N, Zero, ISD::SETNE));
+ return;
+ }
+ setValue(&I, DAG.getNode(ISD::TRUNCATE, DestVT, N));
+}
This is wrong, trunc to bool does a trunc, not a comparison against
zero.
+void SelectionDAGLowering::visitFPToUI(User &I) {
+ // FPToUI is never a no-op cast, no need to check
+ SDOperand N = getValue(I.getOperand(0));
+ MVT::ValueType DestVT = TLI.getValueType(I.getType());
+ if (DestVT == MVT::i1) {
+ // Cast to bool is a comparison against zero, not truncation to
zero.
+ SDOperand Zero = DAG.getConstantFP(0.0, N.getValueType());
+ setValue(&I, DAG.getSetCC(MVT::i1, N, Zero, ISD::SETNE));
+ return;
+ }
+ setValue(&I, DAG.getNode(ISD::FP_TO_UINT, DestVT, N));
+}
likewise.
+void SelectionDAGLowering::visitFPToSI(User &I) {
+ // FPToSI is never a no-op cast, no need to check
+ SDOperand N = getValue(I.getOperand(0));
+ MVT::ValueType DestVT = TLI.getValueType(I.getType());
+ if (DestVT == MVT::i1) {
+ // Cast to bool is a comparison against zero, not truncation to
zero.
+ SDOperand Zero = DAG.getConstantFP(0.0, N.getValueType());
+ setValue(&I, DAG.getSetCC(MVT::i1, N, Zero, ISD::SETNE));
+ return;
+ }
+ setValue(&I, DAG.getNode(ISD::FP_TO_SINT, DestVT, N));
+}
likewise.
+void SelectionDAGLowering::visitBitCast(User &I) {
+ SDOperand N = getValue(I.getOperand(0));
+ MVT::ValueType DestVT = TLI.getValueType(I.getType());
my reading of this code says "handle vectors correctly, otherwise
it's a noop cast". This is incorrect, it should build a BIT_CONVERT
node to handle things like float -> int. Replace the code at the end
with something like:
if (DestVT != N.getValueType())
+ setValue(&I, DAG.getNode(ISD::BIT_CONVERT, DestVT, N);
else
+ setValue(&I, N); // noop cast.
-static bool OptimizeNoopCopyExpression(CastInst *CI) {
+static bool OptimizeNoopCopyExpression(ConvertInst *CI) {
...
InsertedCast =
- new CastInst(CI->getOperand(0), CI->getType(), "", InsertPt);
+ ConvertInst::getCast(CI->getOperand(0), CI->getType(), "",
InsertPt);
As before, this is totally unsafe/buggy/invalid/incorrect, as are the
other uses of ConvertInst::getCast in this file.
Index: lib/ExecutionEngine/ExecutionEngine.cpp
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/ExecutionEngine/ExecutionEngine.cpp,v
retrieving revision 1.87
diff -t -d -u -p -5 -r1.87 ExecutionEngine.cpp
In ExecutionEngine::getConstantValue, please emulate previous
behavior more precisely to avoid regressions in the interpreter
(which is not well tested).
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/ExecutionEngine/Interpreter/
Execution.cpp,v
retrieving revision 1.147
diff -t -d -u -p -5 -r1.147 Execution.cpp
--- lib/ExecutionEngine/Interpreter/Execution.cpp 8 Nov 2006 19:16:44
-0000 1.147
+++ lib/ExecutionEngine/Interpreter/Execution.cpp 12 Nov 2006
06:51:40 -0000
@@ -79,12 +79,23 @@ static GenericValue executeSelectInst(Ge
Interpreter::executeConvertOperation is completely wrong. You need
to ensure you're getting the right operation, not something based on
the sign of the type.
diff -t -d -u -p -5 -r1.281 Writer.cpp
--- lib/Target/CBackend/Writer.cpp 8 Nov 2006 06:47:33 -0000 1.281
+++ lib/Target/CBackend/Writer.cpp 12 Nov 2006 06:51:44 -0000
@@ -135,10 +135,11 @@ namespace {
+void CWriter::printCast(unsigned opc, const Type* SrcTy, const Type*
DstTy) {
This code is incorrect for bitcast and is incorrect for FPToUI/FPToSI
when the dest type isn't the right sign.
CWriter::printConstant is needs to have logic similar to printCast.
I have to run, but I left off at lib/Transforms/ExprTypeConvert.cpp.
-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061112/a896ec49/attachment.html>
More information about the llvm-commits
mailing list