From 8896c0fa800d43873e72c4a02e08f50dd4edf8f1 Mon Sep 17 00:00:00 2001 From: Johannes Doerfert Date: Wed, 4 Sep 2019 16:26:20 +0000 Subject: [PATCH] [Attributor] Use the white list for attributes consistently Summary: We create attributes on-demand so we need to check the white list on-demand. This also unifies the location at which we create, initialize, and eventually invalidate new abstract attributes. The tests show mixed results, a few more call site attributes are determined which can cause more iterations. Reviewers: uenoku, sstefan1 Subscribers: hiraditya, bollu, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D66913 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@370922 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Transforms/IPO/Attributor.h | 97 +++++++++++++------ lib/Transforms/IPO/Attributor.cpp | 92 ++++++------------ test/Transforms/FunctionAttrs/align.ll | 4 +- .../Transforms/FunctionAttrs/arg_nocapture.ll | 4 +- .../FunctionAttrs/dereferenceable.ll | 2 +- test/Transforms/FunctionAttrs/liveness.ll | 4 +- .../FunctionAttrs/nofree-attributor.ll | 2 +- test/Transforms/FunctionAttrs/nonnull.ll | 2 +- .../FunctionAttrs/noreturn_async.ll | 2 +- test/Transforms/FunctionAttrs/nosync.ll | 2 +- test/Transforms/FunctionAttrs/nounwind.ll | 2 +- .../read_write_returned_arguments_scc.ll | 2 +- test/Transforms/FunctionAttrs/willreturn.ll | 2 +- 13 files changed, 111 insertions(+), 106 deletions(-) diff --git a/include/llvm/Transforms/IPO/Attributor.h b/include/llvm/Transforms/IPO/Attributor.h index 6aa630a4e79..4e217745ce3 100644 --- a/include/llvm/Transforms/IPO/Attributor.h +++ b/include/llvm/Transforms/IPO/Attributor.h @@ -606,8 +606,11 @@ struct Attributor { /// the abstract attributes. /// \param DepRecomputeInterval Number of iterations until the dependences /// between abstract attributes are recomputed. - Attributor(InformationCache &InfoCache, unsigned DepRecomputeInterval) - : InfoCache(InfoCache), DepRecomputeInterval(DepRecomputeInterval) {} + /// \param Whitelist If not null, a set limiting the attribute opportunities. + Attributor(InformationCache &InfoCache, unsigned DepRecomputeInterval, + DenseSet *Whitelist = nullptr) + : InfoCache(InfoCache), DepRecomputeInterval(DepRecomputeInterval), + Whitelist(Whitelist) {} ~Attributor() { DeleteContainerPointers(AllAbstractAttributes); } @@ -642,33 +645,7 @@ struct Attributor { template const AAType &getAAFor(const AbstractAttribute &QueryingAA, const IRPosition &IRP, bool TrackDependence = true) { - static_assert(std::is_base_of::value, - "Cannot query an attribute with a type not derived from " - "'AbstractAttribute'!"); - - // Lookup the abstract attribute of type AAType. If found, return it after - // registering a dependence of QueryingAA on the one returned attribute. - const auto &KindToAbstractAttributeMap = - AAMap.lookup(const_cast(IRP)); - if (AAType *AA = static_cast( - KindToAbstractAttributeMap.lookup(&AAType::ID))) { - // Do not registr a dependence on an attribute with an invalid state. - if (TrackDependence && AA->getState().isValidState()) - QueryMap[AA].insert(const_cast(&QueryingAA)); - return *AA; - } - - // No matching attribute found, create one. - auto &AA = AAType::createForPosition(IRP, *this); - registerAA(AA); - - // Bootstrap the new attribute with an initial update to propagate - // information, e.g., function -> call site. - AA.update(*this); - - if (TrackDependence && AA.getState().isValidState()) - QueryMap[&AA].insert(const_cast(&QueryingAA)); - return AA; + return getOrCreateAAFor(IRP, &QueryingAA, TrackDependence); } /// Explicitly record a dependence from \p FromAA to \p ToAA, that is if @@ -709,15 +686,13 @@ struct Attributor { /// abstract attribute objects for them. /// /// \param F The function that is checked for attribute opportunities. - /// \param Whitelist If not null, a set limiting the attribute opportunities. /// /// Note that abstract attribute instances are generally created even if the /// IR already contains the information they would deduce. The most important /// reason for this is the single interface, the one of the abstract attribute /// instance, which can be queried without the need to look at the IR in /// various places. - void identifyDefaultAbstractAttributes( - Function &F, DenseSet *Whitelist = nullptr); + void identifyDefaultAbstractAttributes(Function &F); /// Record that \p I is deleted after information was manifested. void deleteAfterManifest(Instruction &I) { ToBeDeletedInsts.insert(&I); } @@ -793,6 +768,61 @@ struct Attributor { const DataLayout &getDataLayout() const { return InfoCache.DL; } private: + + /// The private version of getAAFor that allows to omit a querying abstract + /// attribute. See also the public getAAFor method. + template + const AAType &getOrCreateAAFor(const IRPosition &IRP, + const AbstractAttribute *QueryingAA = nullptr, + bool TrackDependence = false) { + if (const AAType *AAPtr = + lookupAAFor(IRP, QueryingAA, TrackDependence)) + return *AAPtr; + + // No matching attribute found, create one. + // Use the static create method. + auto &AA = AAType::createForPosition(IRP, *this); + registerAA(AA); + AA.initialize(*this); + + // Bootstrap the new attribute with an initial update to propagate + // information, e.g., function -> call site. If it is not on a given + // whitelist we will not perform updates at all. + if (Whitelist && !Whitelist->count(&AAType::ID)) + AA.getState().indicatePessimisticFixpoint(); + else + AA.update(*this); + + if (TrackDependence && AA.getState().isValidState()) + QueryMap[&AA].insert(const_cast(QueryingAA)); + return AA; + } + + /// Return the attribute of \p AAType for \p IRP if existing. + template + const AAType *lookupAAFor(const IRPosition &IRP, + const AbstractAttribute *QueryingAA = nullptr, + bool TrackDependence = false) { + static_assert(std::is_base_of::value, + "Cannot query an attribute with a type not derived from " + "'AbstractAttribute'!"); + assert((QueryingAA || !TrackDependence) && + "Cannot track dependences without a QueryingAA!"); + + // Lookup the abstract attribute of type AAType. If found, return it after + // registering a dependence of QueryingAA on the one returned attribute. + const auto &KindToAbstractAttributeMap = + AAMap.lookup(const_cast(IRP)); + if (AAType *AA = static_cast( + KindToAbstractAttributeMap.lookup(&AAType::ID))) { + // Do not register a dependence on an attribute with an invalid state. + if (TrackDependence && AA->getState().isValidState()) + QueryMap[AA].insert(const_cast(QueryingAA)); + return AA; + } + return nullptr; + } + /// The set of all abstract attributes. ///{ using AAVector = SmallVector; @@ -823,6 +853,9 @@ private: /// recomputed. const unsigned DepRecomputeInterval; + /// If not null, a set limiting the attribute opportunities. + const DenseSet *Whitelist; + /// Functions, blocks, and instructions we delete after manifest is done. /// ///{ diff --git a/lib/Transforms/IPO/Attributor.cpp b/lib/Transforms/IPO/Attributor.cpp index 580d8e32692..7dd07404614 100644 --- a/lib/Transforms/IPO/Attributor.cpp +++ b/lib/Transforms/IPO/Attributor.cpp @@ -1790,16 +1790,8 @@ struct AAIsDeadImpl : public AAIsDead { void initialize(Attributor &A) override { const Function *F = getAssociatedFunction(); - - if (F->hasInternalLinkage()) - return; - - if (!F || !F->hasExactDefinition()) { - indicatePessimisticFixpoint(); - return; - } - - exploreFromEntry(A, F); + if (F && !F->isDeclaration()) + exploreFromEntry(A, F); } void exploreFromEntry(Attributor &A, const Function *F) { @@ -2355,6 +2347,11 @@ struct AAAlignImpl : AAAlign { getAttrs({Attribute::Alignment}, Attrs); for (const Attribute &Attr : Attrs) takeKnownMaximum(Attr.getValueAsInt()); + + if (getIRPosition().isFnInterfaceKind() && + (!getAssociatedFunction() || + !getAssociatedFunction()->hasExactDefinition())) + indicatePessimisticFixpoint(); } /// See AbstractAttribute::manifest(...). @@ -3311,70 +3308,52 @@ ChangeStatus Attributor::run() { return ManifestChange; } -/// Helper function that checks if an abstract attribute of type \p AAType -/// should be created for IR position \p IRP and if so creates and registers it -/// with the Attributor \p A. -/// -/// This method will look at the provided whitelist. If one is given and the -/// kind \p AAType::ID is not contained, no abstract attribute is created. -/// -/// \returns The created abstract argument, or nullptr if none was created. -template -static const AAType *checkAndRegisterAA(const IRPosition &IRP, Attributor &A, - DenseSet *Whitelist) { - if (Whitelist && !Whitelist->count(&AAType::ID)) - return nullptr; - - return &A.registerAA(*new AAType(IRP)); -} - -void Attributor::identifyDefaultAbstractAttributes( - Function &F, DenseSet *Whitelist) { +void Attributor::identifyDefaultAbstractAttributes(Function &F) { IRPosition FPos = IRPosition::function(F); // Check for dead BasicBlocks in every function. // We need dead instruction detection because we do not want to deal with // broken IR in which SSA rules do not apply. - checkAndRegisterAA(FPos, *this, /* Whitelist */ nullptr); + getOrCreateAAFor(FPos); // Every function might be "will-return". - checkAndRegisterAA(FPos, *this, Whitelist); + getOrCreateAAFor(FPos); // Every function can be nounwind. - checkAndRegisterAA(FPos, *this, Whitelist); + getOrCreateAAFor(FPos); // Every function might be marked "nosync" - checkAndRegisterAA(FPos, *this, Whitelist); + getOrCreateAAFor(FPos); // Every function might be "no-free". - checkAndRegisterAA(FPos, *this, Whitelist); + getOrCreateAAFor(FPos); // Every function might be "no-return". - checkAndRegisterAA(FPos, *this, Whitelist); + getOrCreateAAFor(FPos); // Return attributes are only appropriate if the return type is non void. Type *ReturnType = F.getReturnType(); if (!ReturnType->isVoidTy()) { // Argument attribute "returned" --- Create only one per function even // though it is an argument attribute. - checkAndRegisterAA(FPos, *this, Whitelist); + getOrCreateAAFor(FPos); if (ReturnType->isPointerTy()) { IRPosition RetPos = IRPosition::returned(F); // Every function with pointer return type might be marked align. - checkAndRegisterAA(RetPos, *this, Whitelist); + getOrCreateAAFor(RetPos); // Every function with pointer return type might be marked nonnull. - checkAndRegisterAA(RetPos, *this, Whitelist); + getOrCreateAAFor(RetPos); // Every function with pointer return type might be marked noalias. - checkAndRegisterAA(RetPos, *this, Whitelist); + getOrCreateAAFor(RetPos); // Every function with pointer return type might be marked // dereferenceable. - checkAndRegisterAA(RetPos, *this, Whitelist); + getOrCreateAAFor(RetPos); } } @@ -3382,19 +3361,19 @@ void Attributor::identifyDefaultAbstractAttributes( if (Arg.getType()->isPointerTy()) { IRPosition ArgPos = IRPosition::argument(Arg); // Every argument with pointer type might be marked nonnull. - checkAndRegisterAA(ArgPos, *this, Whitelist); + getOrCreateAAFor(ArgPos); // Every argument with pointer type might be marked noalias. - checkAndRegisterAA(ArgPos, *this, Whitelist); + getOrCreateAAFor(ArgPos); // Every argument with pointer type might be marked dereferenceable. - checkAndRegisterAA(ArgPos, *this, Whitelist); + getOrCreateAAFor(ArgPos); // Every argument with pointer type might be marked align. - checkAndRegisterAA(ArgPos, *this, Whitelist); + getOrCreateAAFor(ArgPos); // Every argument with pointer type might be marked nocapture. - checkAndRegisterAA(ArgPos, *this, Whitelist); + getOrCreateAAFor(ArgPos); } } @@ -3420,15 +3399,13 @@ void Attributor::identifyDefaultAbstractAttributes( break; case Instruction::Load: // The alignment of a pointer is interesting for loads. - checkAndRegisterAA( - IRPosition::value(*cast(I).getPointerOperand()), *this, - Whitelist); + getOrCreateAAFor( + IRPosition::value(*cast(I).getPointerOperand())); break; case Instruction::Store: // The alignment of a pointer is interesting for stores. - checkAndRegisterAA( - IRPosition::value(*cast(I).getPointerOperand()), *this, - Whitelist); + getOrCreateAAFor( + IRPosition::value(*cast(I).getPointerOperand())); break; case Instruction::Call: case Instruction::CallBr: @@ -3452,19 +3429,16 @@ void Attributor::identifyDefaultAbstractAttributes( IRPosition CSArgPos = IRPosition::callsite_argument(CS, i); // Call site argument attribute "non-null". - checkAndRegisterAA(CSArgPos, *this, - Whitelist); + getOrCreateAAFor(CSArgPos); // Call site argument attribute "no-alias". - checkAndRegisterAA(CSArgPos, *this, - Whitelist); + getOrCreateAAFor(CSArgPos); // Call site argument attribute "dereferenceable". - checkAndRegisterAA(CSArgPos, *this, - Whitelist); + getOrCreateAAFor(CSArgPos); // Call site argument attribute "align". - checkAndRegisterAA(CSArgPos, *this, Whitelist); + getOrCreateAAFor(CSArgPos); } } } @@ -3633,7 +3607,6 @@ const char AANoCapture::ID = 0; SWITCH_PK_CREATE(CLASS, IRP, IRP_FUNCTION, Function) \ SWITCH_PK_CREATE(CLASS, IRP, IRP_CALL_SITE, CallSite) \ } \ - AA->initialize(A); \ return *AA; \ } @@ -3650,7 +3623,6 @@ const char AANoCapture::ID = 0; SWITCH_PK_CREATE(CLASS, IRP, IRP_CALL_SITE_RETURNED, CallSiteReturned) \ SWITCH_PK_CREATE(CLASS, IRP, IRP_CALL_SITE_ARGUMENT, CallSiteArgument) \ } \ - AA->initialize(A); \ return *AA; \ } diff --git a/test/Transforms/FunctionAttrs/align.ll b/test/Transforms/FunctionAttrs/align.ll index 269f5e71eb9..b70c69b42d8 100644 --- a/test/Transforms/FunctionAttrs/align.ll +++ b/test/Transforms/FunctionAttrs/align.ll @@ -1,4 +1,4 @@ -; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=6 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR +; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=7 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" @@ -88,7 +88,7 @@ define internal i8* @f1(i8* readnone %0) local_unnamed_addr #0 { br i1 %2, label %3, label %5 ;