From 404eb05247e442d105accb2a7d17a2d4e6f2462f Mon Sep 17 00:00:00 2001 From: Duncan Sands Date: Sun, 6 Jan 2008 18:27:01 +0000 Subject: [PATCH] The transform that tries to turn calls to bitcast functions into direct calls bails out unless caller and callee have essentially equivalent parameter attributes. This is illogical - the callee's attributes should be of no relevance here. Rework the logic, which incidentally fixes a crash when removed arguments have attributes. llvm-svn: 45658 --- llvm/include/llvm/ParameterAttributes.h | 21 ++----- .../IPO/DeadArgumentElimination.cpp | 5 +- .../Scalar/InstructionCombining.cpp | 55 +++++++++++++--- llvm/lib/VMCore/ParameterAttributes.cpp | 63 ++++++------------- llvm/lib/VMCore/Verifier.cpp | 20 ++---- .../2008-01-06-BitCastAttributes.ll | 23 +++++++ 6 files changed, 97 insertions(+), 90 deletions(-) create mode 100644 llvm/test/Transforms/InstCombine/2008-01-06-BitCastAttributes.ll diff --git a/llvm/include/llvm/ParameterAttributes.h b/llvm/include/llvm/ParameterAttributes.h index fc0f96e03607..b66f991084c3 100644 --- a/llvm/include/llvm/ParameterAttributes.h +++ b/llvm/include/llvm/ParameterAttributes.h @@ -22,6 +22,8 @@ #include namespace llvm { +class Type; + namespace ParamAttr { /// Function parameters and results can have attributes to indicate how they @@ -44,13 +46,6 @@ enum Attributes { ReadOnly = 1 << 10 ///< Function only reads from memory }; -/// These attributes can safely be dropped from a function or a function call: -/// doing so may reduce the number of optimizations performed, but it will not -/// change a correct program into an incorrect one. -/// @brief Attributes that do not change the calling convention. -const uint16_t Informative = NoReturn | NoUnwind | NoAlias | - ReadNone | ReadOnly; - /// @brief Attributes that only apply to function parameters. const uint16_t ParameterOnly = ByVal | InReg | Nest | StructRet; @@ -63,10 +58,6 @@ const uint16_t IntegerTypeOnly = SExt | ZExt; /// @brief Attributes that only apply to pointers. const uint16_t PointerTypeOnly = ByVal | Nest | NoAlias | StructRet; -/// @brief Attributes that do not apply to void type function return values. -const uint16_t VoidTypeIncompatible = IntegerTypeOnly | PointerTypeOnly | - ParameterOnly; - /// @brief Attributes that are mutually incompatible. const uint16_t MutuallyIncompatible[3] = { ByVal | InReg | Nest | StructRet, @@ -74,6 +65,9 @@ const uint16_t MutuallyIncompatible[3] = { ReadNone | ReadOnly }; +/// @brief Which of the given attributes do not apply to the type. +uint16_t incompatibleWithType (const Type *Ty, uint16_t attrs); + } // end namespace ParamAttr /// This is just a pair of values to associate a set of parameter attributes @@ -158,11 +152,6 @@ class ParamAttrsList : public FoldingSetNode { static const ParamAttrsList *excludeAttrs(const ParamAttrsList *PAL, uint16_t idx, uint16_t attrs); - /// Returns whether each of the specified lists of attributes can be safely - /// replaced with the other in a function or a function call. - /// @brief Whether one attribute list can safely replace the other. - static bool areCompatible(const ParamAttrsList *A, const ParamAttrsList *B); - /// @} /// @name Accessors /// @{ diff --git a/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp b/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp index 94ae404107b2..8e6a3b91c1f4 100644 --- a/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp +++ b/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp @@ -505,7 +505,7 @@ void DAE::RemoveDeadArgumentsFromFunction(Function *F) { const Type *RetTy = FTy->getReturnType(); if (DeadRetVal.count(F)) { RetTy = Type::VoidTy; - RAttrs &= ~ParamAttr::VoidTypeIncompatible; + RAttrs &= ~ParamAttr::incompatibleWithType(RetTy, RAttrs); DeadRetVal.erase(F); } @@ -561,8 +561,7 @@ void DAE::RemoveDeadArgumentsFromFunction(Function *F) { // The call return attributes. uint16_t RAttrs = PAL ? PAL->getParamAttrs(0) : 0; // Adjust in case the function was changed to return void. - if (NF->getReturnType() == Type::VoidTy) - RAttrs &= ~ParamAttr::VoidTypeIncompatible; + RAttrs &= ~ParamAttr::incompatibleWithType(NF->getReturnType(), RAttrs); if (RAttrs) ParamAttrsVec.push_back(ParamAttrsWithIndex::get(0, RAttrs)); diff --git a/llvm/lib/Transforms/Scalar/InstructionCombining.cpp b/llvm/lib/Transforms/Scalar/InstructionCombining.cpp index 4fcd47a870c1..3d37bcd5890d 100644 --- a/llvm/lib/Transforms/Scalar/InstructionCombining.cpp +++ b/llvm/lib/Transforms/Scalar/InstructionCombining.cpp @@ -8074,6 +8074,7 @@ bool InstCombiner::transformConstExprCastCall(CallSite CS) { return false; Function *Callee = cast(CE->getOperand(0)); Instruction *Caller = CS.getInstruction(); + const ParamAttrsList* CallerPAL = CS.getParamAttrs(); // Okay, this is a cast from a function to a different type. Unless doing so // would cause a type conversion of one of our arguments, change this call to @@ -8082,13 +8083,6 @@ bool InstCombiner::transformConstExprCastCall(CallSite CS) { const FunctionType *FT = Callee->getFunctionType(); const Type *OldRetTy = Caller->getType(); - const ParamAttrsList* CallerPAL = CS.getParamAttrs(); - - // If the parameter attributes are not compatible, don't do the xform. We - // don't want to lose an sret attribute or something. - if (!ParamAttrsList::areCompatible(CallerPAL, Callee->getParamAttrs())) - return false; - // Check to see if we are changing the return type... if (OldRetTy != FT->getReturnType()) { if (Callee->isDeclaration() && !Caller->use_empty() && @@ -8103,6 +8097,11 @@ bool InstCombiner::transformConstExprCastCall(CallSite CS) { FT->getReturnType() != Type::VoidTy) return false; // Cannot transform this return value. + if (!Caller->use_empty() && CallerPAL && + ParamAttr::incompatibleWithType(FT->getReturnType(), + CallerPAL->getParamAttrs(0))) + return false; // Attribute not compatible with transformed value. + // If the callsite is an invoke instruction, and the return value is used by // a PHI node in a successor, we cannot change the return type of the call // because there is no place to put the cast instruction (without breaking @@ -8126,7 +8125,11 @@ bool InstCombiner::transformConstExprCastCall(CallSite CS) { const Type *ActTy = (*AI)->getType(); if (!CastInst::isCastable(ActTy, ParamTy)) - return false; + return false; // Cannot transform this parameter value. + + if (CallerPAL && + ParamAttr::incompatibleWithType(ParamTy, CallerPAL->getParamAttrs(i+1))) + return false; // Attribute not compatible with transformed value. ConstantInt *c = dyn_cast(*AI); // Some conversions are safe even if we do not have a body. @@ -8144,10 +8147,32 @@ bool InstCombiner::transformConstExprCastCall(CallSite CS) { Callee->isDeclaration()) return false; // Do not delete arguments unless we have a function body... + if (FT->getNumParams() < NumActualArgs && FT->isVarArg()) + // In this case we have more arguments than the new function type, but we + // won't be dropping them. Some of them may have attributes. If so, we + // cannot perform the transform because attributes are not allowed after + // the end of the function type. + if (CallerPAL && CallerPAL->size() && + CallerPAL->getParamIndex(CallerPAL->size()-1) > FT->getNumParams()) + return false; + // Okay, we decided that this is a safe thing to do: go ahead and start // inserting cast instructions as necessary... std::vector Args; Args.reserve(NumActualArgs); + ParamAttrsVector attrVec; + attrVec.reserve(NumCommonArgs); + + // Get any return attributes. + uint16_t RAttrs = CallerPAL ? CallerPAL->getParamAttrs(0) : 0; + + // If the return value is not being used, the type may not be compatible + // with the existing attributes. Wipe out any problematic attributes. + RAttrs &= ~ParamAttr::incompatibleWithType(FT->getReturnType(), RAttrs); + + // Add the new return attributes. + if (RAttrs) + attrVec.push_back(ParamAttrsWithIndex::get(0, RAttrs)); AI = CS.arg_begin(); for (unsigned i = 0; i != NumCommonArgs; ++i, ++AI) { @@ -8160,6 +8185,11 @@ bool InstCombiner::transformConstExprCastCall(CallSite CS) { CastInst *NewCast = CastInst::create(opcode, *AI, ParamTy, "tmp"); Args.push_back(InsertNewInstBefore(NewCast, *Caller)); } + + // Add any parameter attributes. + uint16_t PAttrs = CallerPAL ? CallerPAL->getParamAttrs(i + 1) : 0; + if (PAttrs) + attrVec.push_back(ParamAttrsWithIndex::get(i + 1, PAttrs)); } // If the function takes more arguments than the call was taking, add them @@ -8187,17 +8217,22 @@ bool InstCombiner::transformConstExprCastCall(CallSite CS) { Args.push_back(*AI); } } + + // No need to add parameter attributes - we already checked that there + // aren't any. } if (FT->getReturnType() == Type::VoidTy) Caller->setName(""); // Void type should not have a name. + const ParamAttrsList* NewCallerPAL = ParamAttrsList::get(attrVec); + Instruction *NC; if (InvokeInst *II = dyn_cast(Caller)) { NC = new InvokeInst(Callee, II->getNormalDest(), II->getUnwindDest(), Args.begin(), Args.end(), Caller->getName(), Caller); cast(NC)->setCallingConv(II->getCallingConv()); - cast(NC)->setParamAttrs(CallerPAL); + cast(NC)->setParamAttrs(NewCallerPAL); } else { NC = new CallInst(Callee, Args.begin(), Args.end(), Caller->getName(), Caller); @@ -8205,7 +8240,7 @@ bool InstCombiner::transformConstExprCastCall(CallSite CS) { if (CI->isTailCall()) cast(NC)->setTailCall(); cast(NC)->setCallingConv(CI->getCallingConv()); - cast(NC)->setParamAttrs(CallerPAL); + cast(NC)->setParamAttrs(NewCallerPAL); } // Insert a cast of the return type as necessary. diff --git a/llvm/lib/VMCore/ParameterAttributes.cpp b/llvm/lib/VMCore/ParameterAttributes.cpp index a95e663337da..840d54b6323f 100644 --- a/llvm/lib/VMCore/ParameterAttributes.cpp +++ b/llvm/lib/VMCore/ParameterAttributes.cpp @@ -7,11 +7,12 @@ // //===----------------------------------------------------------------------===// // -// This file implements the ParamAttrsList class. +// This file implements the ParamAttrsList class and ParamAttr utilities. // //===----------------------------------------------------------------------===// #include "llvm/ParameterAttributes.h" +#include "llvm/DerivedTypes.h" #include "llvm/Support/ManagedStatic.h" using namespace llvm; @@ -63,50 +64,6 @@ ParamAttrsList::getParamAttrsText(uint16_t Attrs) { return Result; } -/// onlyInformative - Returns whether only informative attributes are set. -static inline bool onlyInformative(uint16_t attrs) { - return !(attrs & ~ParamAttr::Informative); -} - -bool -ParamAttrsList::areCompatible(const ParamAttrsList *A, const ParamAttrsList *B){ - if (A == B) - return true; - unsigned ASize = A ? A->size() : 0; - unsigned BSize = B ? B->size() : 0; - unsigned AIndex = 0; - unsigned BIndex = 0; - - while (AIndex < ASize && BIndex < BSize) { - uint16_t AIdx = A->getParamIndex(AIndex); - uint16_t BIdx = B->getParamIndex(BIndex); - uint16_t AAttrs = A->getParamAttrsAtIndex(AIndex); - uint16_t BAttrs = B->getParamAttrsAtIndex(AIndex); - - if (AIdx < BIdx) { - if (!onlyInformative(AAttrs)) - return false; - ++AIndex; - } else if (BIdx < AIdx) { - if (!onlyInformative(BAttrs)) - return false; - ++BIndex; - } else { - if (!onlyInformative(AAttrs ^ BAttrs)) - return false; - ++AIndex; - ++BIndex; - } - } - for (; AIndex < ASize; ++AIndex) - if (!onlyInformative(A->getParamAttrsAtIndex(AIndex))) - return false; - for (; BIndex < BSize; ++BIndex) - if (!onlyInformative(B->getParamAttrsAtIndex(AIndex))) - return false; - return true; -} - void ParamAttrsList::Profile(FoldingSetNodeID &ID, const ParamAttrsVector &Attrs) { for (unsigned i = 0; i < Attrs.size(); ++i) @@ -229,3 +186,19 @@ ParamAttrsList::excludeAttrs(const ParamAttrsList *PAL, return getModified(PAL, modVec); } +uint16_t ParamAttr::incompatibleWithType (const Type *Ty, uint16_t attrs) { + uint16_t Incompatible = None; + + if (!Ty->isInteger()) + Incompatible |= IntegerTypeOnly; + + if (!isa(Ty)) + Incompatible |= PointerTypeOnly; + else if (attrs & ParamAttr::ByVal) { + const PointerType *PTy = cast(Ty); + if (!isa(PTy->getElementType())) + Incompatible |= ParamAttr::ByVal; + } + + return attrs & Incompatible; +} diff --git a/llvm/lib/VMCore/Verifier.cpp b/llvm/lib/VMCore/Verifier.cpp index d305855b436e..95f871be339d 100644 --- a/llvm/lib/VMCore/Verifier.cpp +++ b/llvm/lib/VMCore/Verifier.cpp @@ -418,22 +418,10 @@ void Verifier::VerifyParamAttrs(const FunctionType *FT, Attrs->getParamAttrsText(MutI) + "are incompatible!", V); } - uint16_t IType = Attr & ParamAttr::IntegerTypeOnly; - Assert1(!IType || FT->getParamType(Idx-1)->isInteger(), - "Attribute " + Attrs->getParamAttrsText(IType) + - "should only apply to Integer type!", V); - - uint16_t PType = Attr & ParamAttr::PointerTypeOnly; - Assert1(!PType || isa(FT->getParamType(Idx-1)), - "Attribute " + Attrs->getParamAttrsText(PType) + - "should only apply to Pointer type!", V); - - if (Attr & ParamAttr::ByVal) { - const PointerType *Ty = - dyn_cast(FT->getParamType(Idx-1)); - Assert1(!Ty || isa(Ty->getElementType()), - "Attribute byval should only apply to pointer to structs!", V); - } + uint16_t IType = ParamAttr::incompatibleWithType(FT->getParamType(Idx-1), + Attr); + Assert1(!IType, "Wrong type for attribute " + + Attrs->getParamAttrsText(IType), V); if (Attr & ParamAttr::Nest) { Assert1(!SawNest, "More than one parameter has attribute nest!", V); diff --git a/llvm/test/Transforms/InstCombine/2008-01-06-BitCastAttributes.ll b/llvm/test/Transforms/InstCombine/2008-01-06-BitCastAttributes.ll new file mode 100644 index 000000000000..5bb8875961d5 --- /dev/null +++ b/llvm/test/Transforms/InstCombine/2008-01-06-BitCastAttributes.ll @@ -0,0 +1,23 @@ +; RUN: llvm-as < %s | opt -instcombine | llvm-dis | grep bitcast | count 2 + +define void @a() { + ret void +} + +define i32 @b(i32* inreg %x) signext { + ret i32 0 +} + +define void @c(...) { + ret void +} + +define void @g(i32* %y) { + call void bitcast (void ()* @a to void (i32*)*)( i32* noalias %y ) + call <2 x i32> bitcast (i32 (i32*)* @b to <2 x i32> (i32*)*)( i32* inreg null ) ; <<2 x i32>>:1 [#uses=0] + %x = call i64 bitcast (i32 (i32*)* @b to i64 (i32)*)( i32 0 ) ; [#uses=0] + call void bitcast (void (...)* @c to void (i32)*)( i32 0 ) + call i32 bitcast (i32 (i32*)* @b to i32 (i32)*)( i32 zeroext 0 ) ; :2 [#uses=0] + call void bitcast (void (...)* @c to void (i32)*)( i32 zeroext 0 ) + ret void +}