[llvm-commits] CAST Patch Update [1/4] first third of the patch.

[Reid Spencer]

On Sun, 2006-11-12 at 17:51 -0800, Chris Lattner wrote:
> > 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.

Understood. I wouldn't anyway.

> 
> > 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?

I doubt it. The front-end patch is pretty simple. 

I think I've tracked the problem down to an issue in InstCombine,
despite previous claims. However, I haven't fully identified it .. just
narrowed a test case (manually).



> Meta issues:
> 
> 
> Please send the llvm-gcc patch as well.

Done.

> I realize Sheng is probably working on it, but plz make sure the
> ConstantExpr::getUShr patch happens.

It happened with the original commit. getUShr and getSShr don't exist
any more (on mainline).  I went to do this and couldn't find any
instances of it.

> 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 :).

I was thinking version 6, actually.  That corresponds with the last 1.X
release. If they want to move to 2.0 they have to upgrade to 1.9 first
and then to 2.0.  I doubt there's going to be many people doing this.
Its too easy to recreate the bc with 2.0, but version 5 is doable as
well. Note that the bulk of that mess is handling version 5 so going
with version 5 doesn't clean much up.

> 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.

Yes, that was the plan. This is to minimize the impact of the CAST
patch. In a follow on patch, I might add a bool argument to getCast to
indicated signedness. That way the caller can dictate how integers
should be treated when making the cast. Might be simpler to do that than
have every call sight insert SExt/Zext/Trunc instructions or convert
constants to the correct sign.

>   Please remove the spaces from around the arguments.

done.

> 
> --- 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 @@
>  
> 
>  #include "llvm/Instruction.h"
> +#include "llvm/Type.h"
>  
> Please remove this #include, by moving stuff out-of-line as
> appropriate.

How do you move a default argument out of line?  I really don't want to
change all the call sites to pass the defaulted parameter.

> 
> Change "(if its non-null)" -> "(if it is non-null)", multiple
> instances.

done.
> 
> 
> 
> 
> +  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 *.

I'll audit the patch for this and change them all to be next to the
var/param name. Nicholas commented on this too, I just haven't gotten to
it yet since I've been trying to squash bugs and had bigger changes to
push through (inttoptr and ptrtoint, etc.)
> 
> 
> +  /// 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.

I'm probably going to revert the Convert naming before the final patch
back to Cast.  It seems too confusing and Convert is too long :)   It
was handy to distinguish it from CastInst when I started the patch so I
could find all the places that use CastInst by compiler error.  However
the term cast is used so frequently in comments and documentation and
stuff that I really don't feel like searching them all just for a
terminology change. The term will be cast, the instruction will be
CastInst, I'll remove all usage of convert in the next patch revision.
> 
> +  /// 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?

I was preserving old CastInst cold, but you're right. I've removed it.
There was only one call to it anyway.

> 
> +  /// 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>?  

Because PtrToInt and IntToPtr can be no-op casts too, but only if the
size of source and destination types are equal.  bitcast can't do
ptr->int and int->ptr because it makes bitcast inconsistent (rule wise).
We don't know the size of a pointer in most contexts. Not having bitcast
do ptr->int or int->ptr (of the same size) means less condition checking
throughout the code.  In the few places that are looking for ptr->int or
int->ptr they can take the extra step to figure out what an IntPtrTy
should be and pass it to isNoopCast to determine if it really is a noop.


> What is IntPtrTy?  This isn't safe.

IntPtrTy is the integer type that corresponds to a pointer. The existing
implementation assumes it is LongTy for constant expressions. The
default value for the parameter preserves that case.  Elsewhere
(InstCombine) the TargetData is available so it passes the result of
TargetData::getIntPtrType(). Having this allows the right thing to be
done and preserves existing functionality. 

> 
> +  /// @brief Abstract clone() method (delegated to its subclasses).
> +  virtual ConvertInst *clone() const = 0;
> +
> 
> 
> Please just remove this.

Okay .. guess its redundant with Instruction::clone().

> 
> 
> --- 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? 

Yes, it is.

>  In addition, it got renamed.

I've updated the comment.

> 
> --- 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> ?

Because there are many places that don't have an Instruction* that need
to determine whether something is a convert or not (like all the places
handling ConstantExpr of converts).

Note there's both a static and non-static version.  The non-static
version is used by ConstantExpr::isConvert() so that there's only one
implementation.

> ===================================================================
> 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.

Yikes, so it is. Fixed.

> 
> 
> --- 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.

They do. Its hidden in the definition of the macro before
Instruction.def is #included.

> 
> 
> --- 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.

Its dead. Removed it.
> 
> 
> ===================================================================
> 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.

Okay. 

> 
>    } else if (const GlobalValue *GV = dyn_cast<GlobalValue>(V)) {
> +    // FIXME: Isn't this redundant with hasUniqueAddress check above?
>      return GV;
>    }
> 
> 
> Yes, I removed it.

Thanks. That was the intent of the comment but at the time I discovered
it, I wasn't too sure and didn't want to break alias analysis.
> 
> 
> @@ -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);

Done.
> 
> 
> 
> 
> @@ -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.

Done.
> 
> 
> 
> 
>    // 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.

The immediately preceding if statement:

>   if ((!isa<PointerType>(V1->getType()) || !isa<PointerType>(V2->getType())) &&
>       V1->getType() != Type::LongTy && V2->getType() != Type::LongTy)
>     return NoAlias;  // Scalars cannot alias each other

indicates V1 and V2 could be either a LongTy or a PointerTy.  So,
removing the isa<PointerType> check could break this code if one of them
is LongTy, couldn't it?

> 
>    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.

I don't get it. You are asking me to put in a manifest constant value 64
here in the comparisons?  Not that you can't sign extend if the sizes
are equal (invalid cast).
> 
> 
> ===================================================================
> 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.

Okay.
> 
> 
> +    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
> 

Done.
> 
> 
> 
> 
> --- 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? 

The CastInst class did all casting by implication using only the source
and destination type. ConvertInst::getCast() makes all that logic
explicit by selecting the correct opcode based on the types.  I've
checked this with my unit test case. It produces identical results.

>  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). 

Yes, but for now it does. Remember, signed integer types aren't gone
yet. When they disappear, this will break.  The getCast logic is correct
while we have signed integer types.  Once all the instructions are in
and working we will revisit every getCast site (individually
reviewed/committed) and change it back to a new ***Inst(V, Ty, ...)
call, with the correct conversion instruction type. I chose this path to
minimize this first CAST patch. When types become signless, considerable
logic must be added in these kidns of places to determine which kind of
cast to create.  For now, with signed types available, that logic is
available in exactly one place: ConvertInst::getCastOpcode().


>  I think you should totally eliminate ConvertInst::getCast and the
> ConstantExpr version as well.

Yes, when the time comes, I will.
> 
> 
> 
> 
> @@ -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.

There's 100s of these .. I"m aware of the issue and will remove them on
subsequent patches when signed types are closer to going away. Till
then, they stay.
> 
> 
> ===================================================================
> 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 :)

Sorry, but without this value numbering crashes because it starts
claiming identical instructions that aren't identical.  This was one of
the hard bugs to find. It is insufficient to only check the opcode and
destination type.
> 
> 
> 
> 
> 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.

Okay.
> 

> ===================================================================
> 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.

Changed them both to handle only BitCastInst or IntToPtrInst which are
the only two casts that result in pointer types.

> 
> 
> 
> 
> ===================================================================
> 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.

You have said elsewhere (SelectionDAGISel.cpp) that cast to bool should
*not* be interpreted as a setne to 0 (or setne to 0.0). Which is it?

Does this even matter in light of dropping backwards compatibility?

> --- 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.

As before, this works fine while integer types are signed. I'll fix this
when this initial cast is in and the other instructions are done.

> 
> 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.

Yeah? That's preserved logic from existing implementation. I'm not here
to fix every bug but to retain as much existing behavior as possible.
See TOT SelectionDAGISel.cpp lines 1488 through 1492:

>  } else if (DestVT == MVT::i1) {
>     // Cast to bool is a comparison against zero, not truncation to zero.
>     SDOperand Zero = isInteger(SrcVT) ? DAG.getConstant(0, N.getValueType()) :
>                                        DAG.getConstantFP(0.0, N.getValueType());
>     setValue(&I, DAG.getSetCC(MVT::i1, N, Zero, ISD::SETNE));

It did this for *all* casts. I was simply preserving the behavior.  Are
you SURE this needs to be deleted?

> 
> 
> +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.

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.

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. 

Huh? That's FPToUI conversion, handled in the visitFPToUI function.

>  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.
> 

Hmm. I think I get your meaning now (contrary to comment above).  The
case where they want to BITCONVERT (rather than numerically interpret) a
float -> int needs to be handled. I had assumed that setValue(&I, N)
would handle that case but you're suggesting it needs to be a BITCONVERT
node instead. I'll give this a shot and write a test case for it because
it clearly isn't handled in any of our test suites.

On second thought, why is this even legal? Should it be done through a
pointer? 

> 
> -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.

As before, its fine while types are signed.
> 
> 
> 
> 
> 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).

Okay. The only question I have is the bitcast case. The existing logic
will return the generic value if the operand type and destination type
have the same TypeID. This means that ptr->ptr and packed->packed and
int->int will all be fine. Is that what was intended or should it simply
be restricted to ptr->ptr as the comment suggests?  Here's the old code:

> -
> -      // Handle cast of pointer to pointer...
> -      if (Op->getType()->getTypeID() == C->getType()->getTypeID())
> -        return GV;
> -
> -      // Handle a cast of pointer to any integral type...
> -      if (isa<PointerType>(Op->getType()) && C->getType()->isIntegral())
> -        return GV;
> -
> -      // Handle cast of integer to a pointer...

> 
> ===================================================================
> 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.

I guess I missed this because it doesn't refer to "Cast" in any way. Its
wrong because it never got changed for the new cast instructions.  I
will implement the cast conversions and submit with my next revision of
this patch.

> 
> 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.

Why is bitcast wrong? Are you implying that a secondary cast is
necessary for these conversions? bitcast handles:

ptr -> ptr              <<< doesn't require secondary cast, one cast is
enough
integer -> integer      <<< doesn't require secondary cast, no-op (same
type)
packed -> integer       <<< packed not handled by CBE currently
integer -> packed       <<< ditto
integer -> floating     <<< how do you do this in a C expression?
through a union?
floating -> intteger    <<< ditto

The bitcast types must be identical in width otherwise its illegal, as
are:

ptr -> floating
floating -> ptr
integer -> ptr
ptr -> integer

I don't think there's anything wrong with the FPToUI and FPToSI cases.
The code always prints the destination type cast first. The switch
statement is only for a secondary cast. Not all conversion instructions
need a secondary cast. These instructions are such a case. Consider this
example:

fptoui double %x to uint

The code in the patch will (and does) print:

(unsigned int)%x

What more is needed?


> CWriter::printConstant is needs to have logic similar to printCast.

It does, it calls printCast :)

> I have to run, but I left off at lib/Transforms/ExprTypeConvert.cpp.

Thanks for your feedback!

Reid.