Index: llvm/trunk/include/llvm/IR/ModuleSummaryIndex.h =================================================================== --- llvm/trunk/include/llvm/IR/ModuleSummaryIndex.h +++ llvm/trunk/include/llvm/IR/ModuleSummaryIndex.h @@ -106,7 +106,7 @@ /// \brief Sububclass discriminator (for dyn_cast<> et al.) enum SummaryKind : unsigned { AliasKind, FunctionKind, GlobalVarKind }; - /// Group flags (Linkage, noRename, isOptSize, etc.) as a bitfield. + /// Group flags (Linkage, NotEligibleToImport, etc.) as a bitfield. struct GVFlags { /// \brief The linkage type of the associated global value. /// @@ -117,39 +117,14 @@ /// types based on global summary-based analysis. unsigned Linkage : 4; - /// Indicate if the global value cannot be renamed (in a specific section, - /// possibly referenced from inline assembly, etc). - unsigned NoRename : 1; - - /// Indicate if a function contains inline assembly (which is opaque), - /// that may reference a local value. This is used to prevent importing - /// of this function, since we can't promote and rename the uses of the - /// local in the inline assembly. Use a flag rather than bloating the - /// summary with references to every possible local value in the - /// llvm.used set. - unsigned HasInlineAsmMaybeReferencingInternal : 1; - - /// Indicate if the function is not viable to inline. - unsigned IsNotViableToInline : 1; + /// Indicate if the global value cannot be imported (e.g. it cannot + /// be renamed or references something that can't be renamed). + unsigned NotEligibleToImport : 1; /// Convenience Constructors - explicit GVFlags(GlobalValue::LinkageTypes Linkage, bool NoRename, - bool HasInlineAsmMaybeReferencingInternal, - bool IsNotViableToInline) - : Linkage(Linkage), NoRename(NoRename), - HasInlineAsmMaybeReferencingInternal( - HasInlineAsmMaybeReferencingInternal), - IsNotViableToInline(IsNotViableToInline) {} - - GVFlags(const GlobalValue &GV) - : Linkage(GV.getLinkage()), NoRename(GV.hasSection()), - HasInlineAsmMaybeReferencingInternal(false) { - IsNotViableToInline = false; - if (const auto *F = dyn_cast(&GV)) - // Inliner doesn't handle variadic functions. - // FIXME: refactor this to use the same code that inliner is using. - IsNotViableToInline = F->isVarArg(); - } + explicit GVFlags(GlobalValue::LinkageTypes Linkage, + bool NotEligibleToImport) + : Linkage(Linkage), NotEligibleToImport(NotEligibleToImport) {} }; private: @@ -217,31 +192,11 @@ Flags.Linkage = Linkage; } - bool isNotViableToInline() const { return Flags.IsNotViableToInline; } + /// Return true if this global value can't be imported. + bool notEligibleToImport() const { return Flags.NotEligibleToImport; } - /// Return true if this summary is for a GlobalValue that needs promotion - /// to be referenced from another module. - bool needsRenaming() const { return GlobalValue::isLocalLinkage(linkage()); } - - /// Return true if this global value cannot be renamed (in a specific section, - /// possibly referenced from inline assembly, etc). - bool noRename() const { return Flags.NoRename; } - - /// Flag that this global value cannot be renamed (in a specific section, - /// possibly referenced from inline assembly, etc). - void setNoRename() { Flags.NoRename = true; } - - /// Return true if this global value possibly references another value - /// that can't be renamed. - bool hasInlineAsmMaybeReferencingInternal() const { - return Flags.HasInlineAsmMaybeReferencingInternal; - } - - /// Flag that this global value possibly references another value that - /// can't be renamed. - void setHasInlineAsmMaybeReferencingInternal() { - Flags.HasInlineAsmMaybeReferencingInternal = true; - } + /// Flag that this global value cannot be imported. + void setNotEligibleToImport() { Flags.NotEligibleToImport = true; } /// Return the list of values referenced by this global value definition. ArrayRef refs() const { return RefEdgeList; } Index: llvm/trunk/include/llvm/Transforms/Utils/FunctionImportUtils.h =================================================================== --- llvm/trunk/include/llvm/Transforms/Utils/FunctionImportUtils.h +++ llvm/trunk/include/llvm/Transforms/Utils/FunctionImportUtils.h @@ -40,9 +40,20 @@ /// as part of a different backend compilation process. bool HasExportedFunctions = false; + /// Set of llvm.*used values, in order to validate that we don't try + /// to promote any non-renamable values. + SmallPtrSet Used; + /// Check if we should promote the given local value to global scope. bool shouldPromoteLocalToGlobal(const GlobalValue *SGV); +#ifndef NDEBUG + /// Check if the given value is a local that can't be renamed (promoted). + /// Only used in assertion checking, and disabled under NDEBUG since the Used + /// set will not be populated. + bool isNonRenamableLocal(const GlobalValue &GV) const; +#endif + /// Helper methods to check if we are importing from or potentially /// exporting from the current source module. bool isPerformingImport() const { return GlobalsToImport != nullptr; } @@ -82,6 +93,13 @@ // may be exported to another backend compilation. if (!GlobalsToImport) HasExportedFunctions = ImportIndex.hasExportedFunctions(M); + +#ifndef NDEBUG + // First collect those in the llvm.used set. + collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false); + // Next collect those in the llvm.compiler.used set. + collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ true); +#endif } bool run(); Index: llvm/trunk/lib/Analysis/ModuleSummaryAnalysis.cpp =================================================================== --- llvm/trunk/lib/Analysis/ModuleSummaryAnalysis.cpp +++ llvm/trunk/lib/Analysis/ModuleSummaryAnalysis.cpp @@ -80,10 +80,15 @@ return CalleeInfo::HotnessType::None; } -static void computeFunctionSummary(ModuleSummaryIndex &Index, const Module &M, - const Function &F, BlockFrequencyInfo *BFI, - ProfileSummaryInfo *PSI, - bool HasLocalsInUsed) { +static bool isNonRenamableLocal(const GlobalValue &GV) { + return GV.hasSection() && GV.hasLocalLinkage(); +} + +static void +computeFunctionSummary(ModuleSummaryIndex &Index, const Module &M, + const Function &F, BlockFrequencyInfo *BFI, + ProfileSummaryInfo *PSI, bool HasLocalsInUsed, + SmallPtrSet &CantBePromoted) { // Summary not currently supported for anonymous functions, they should // have been named. assert(F.hasName()); @@ -178,34 +183,48 @@ } } - GlobalValueSummary::GVFlags Flags(F); + bool NonRenamableLocal = isNonRenamableLocal(F); + bool NotEligibleForImport = + NonRenamableLocal || HasInlineAsmMaybeReferencingInternal || + // Inliner doesn't handle variadic functions. + // FIXME: refactor this to use the same code that inliner is using. + F.isVarArg(); + GlobalValueSummary::GVFlags Flags(F.getLinkage(), NotEligibleForImport); auto FuncSummary = llvm::make_unique( Flags, NumInsts, RefEdges.takeVector(), CallGraphEdges.takeVector(), TypeTests.takeVector()); - if (HasInlineAsmMaybeReferencingInternal) - FuncSummary->setHasInlineAsmMaybeReferencingInternal(); + if (NonRenamableLocal) + CantBePromoted.insert(F.getGUID()); Index.addGlobalValueSummary(F.getName(), std::move(FuncSummary)); } -static void computeVariableSummary(ModuleSummaryIndex &Index, - const GlobalVariable &V) { +static void +computeVariableSummary(ModuleSummaryIndex &Index, const GlobalVariable &V, + SmallPtrSet &CantBePromoted) { SetVector RefEdges; SmallPtrSet Visited; findRefEdges(&V, RefEdges, Visited); - GlobalValueSummary::GVFlags Flags(V); + bool NonRenamableLocal = isNonRenamableLocal(V); + GlobalValueSummary::GVFlags Flags(V.getLinkage(), NonRenamableLocal); auto GVarSummary = llvm::make_unique(Flags, RefEdges.takeVector()); + if (NonRenamableLocal) + CantBePromoted.insert(V.getGUID()); Index.addGlobalValueSummary(V.getName(), std::move(GVarSummary)); } -static void computeAliasSummary(ModuleSummaryIndex &Index, - const GlobalAlias &A) { - GlobalValueSummary::GVFlags Flags(A); +static void +computeAliasSummary(ModuleSummaryIndex &Index, const GlobalAlias &A, + SmallPtrSet &CantBePromoted) { + bool NonRenamableLocal = isNonRenamableLocal(A); + GlobalValueSummary::GVFlags Flags(A.getLinkage(), NonRenamableLocal); auto AS = llvm::make_unique(Flags, ArrayRef{}); auto *Aliasee = A.getBaseObject(); auto *AliaseeSummary = Index.getGlobalValueSummary(*Aliasee); assert(AliaseeSummary && "Alias expects aliasee summary to be parsed"); AS->setAliasee(AliaseeSummary); + if (NonRenamableLocal) + CantBePromoted.insert(A.getGUID()); Index.addGlobalValueSummary(A.getName(), std::move(AS)); } @@ -226,9 +245,12 @@ collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false); // Next collect those in the llvm.compiler.used set. collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ true); + SmallPtrSet CantBePromoted; for (auto *V : Used) { - if (V->hasLocalLinkage()) + if (V->hasLocalLinkage()) { LocalsUsed.insert(V); + CantBePromoted.insert(V->getGUID()); + } } // Compute summaries for all functions defined in module, and save in the @@ -248,7 +270,8 @@ BFI = BFIPtr.get(); } - computeFunctionSummary(Index, M, F, BFI, PSI, !LocalsUsed.empty()); + computeFunctionSummary(Index, M, F, BFI, PSI, !LocalsUsed.empty(), + CantBePromoted); } // Compute summaries for all variables defined in module, and save in the @@ -256,18 +279,18 @@ for (const GlobalVariable &G : M.globals()) { if (G.isDeclaration()) continue; - computeVariableSummary(Index, G); + computeVariableSummary(Index, G, CantBePromoted); } // Compute summaries for all aliases defined in module, and save in the // index. for (const GlobalAlias &A : M.aliases()) - computeAliasSummary(Index, A); + computeAliasSummary(Index, A, CantBePromoted); for (auto *V : LocalsUsed) { auto *Summary = Index.getGlobalValueSummary(*V); assert(Summary && "Missing summary for global value"); - Summary->setNoRename(); + Summary->setNotEligibleToImport(); } if (!M.getModuleInlineAsm().empty()) { @@ -282,7 +305,8 @@ // referenced from there. ModuleSymbolTable::CollectAsmSymbols( Triple(M.getTargetTriple()), M.getModuleInlineAsm(), - [&M, &Index](StringRef Name, object::BasicSymbolRef::Flags Flags) { + [&M, &Index, &CantBePromoted](StringRef Name, + object::BasicSymbolRef::Flags Flags) { // Symbols not marked as Weak or Global are local definitions. if (Flags & (object::BasicSymbolRef::SF_Weak | object::BasicSymbolRef::SF_Global)) @@ -291,11 +315,9 @@ if (!GV) return; assert(GV->isDeclaration() && "Def in module asm already has definition"); - GlobalValueSummary::GVFlags GVFlags( - GlobalValue::InternalLinkage, - /* NoRename */ true, - /* HasInlineAsmMaybeReferencingInternal */ false, - /* IsNotViableToInline */ true); + GlobalValueSummary::GVFlags GVFlags(GlobalValue::InternalLinkage, + /* NotEligibleToImport */ true); + CantBePromoted.insert(GlobalValue::getGUID(Name)); // Create the appropriate summary type. if (isa(GV)) { std::unique_ptr Summary = @@ -303,18 +325,41 @@ GVFlags, 0, ArrayRef{}, ArrayRef{}, ArrayRef{}); - Summary->setNoRename(); Index.addGlobalValueSummary(Name, std::move(Summary)); } else { std::unique_ptr Summary = llvm::make_unique(GVFlags, ArrayRef{}); - Summary->setNoRename(); Index.addGlobalValueSummary(Name, std::move(Summary)); } }); } + for (auto &GlobalList : Index) { + assert(GlobalList.second.size() == 1 && + "Expected module's index to have one summary per GUID"); + auto &Summary = GlobalList.second[0]; + bool AllRefsCanBeExternallyReferenced = + llvm::all_of(Summary->refs(), [&](const ValueInfo &VI) { + return !CantBePromoted.count(VI.getValue()->getGUID()); + }); + if (!AllRefsCanBeExternallyReferenced) { + Summary->setNotEligibleToImport(); + continue; + } + + if (auto *FuncSummary = dyn_cast(Summary.get())) { + bool AllCallsCanBeExternallyReferenced = llvm::all_of( + FuncSummary->calls(), [&](const FunctionSummary::EdgeTy &Edge) { + auto GUID = Edge.first.isGUID() ? Edge.first.getGUID() + : Edge.first.getValue()->getGUID(); + return !CantBePromoted.count(GUID); + }); + if (!AllCallsCanBeExternallyReferenced) + Summary->setNotEligibleToImport(); + } + } + return Index; } Index: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp =================================================================== --- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp +++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp @@ -801,12 +801,8 @@ // to getDecodedLinkage() will need to be taken into account here as above. auto Linkage = GlobalValue::LinkageTypes(RawFlags & 0xF); // 4 bits RawFlags = RawFlags >> 4; - bool NoRename = RawFlags & 0x1; - bool IsNotViableToInline = RawFlags & 0x2; - bool HasInlineAsmMaybeReferencingInternal = RawFlags & 0x4; - return GlobalValueSummary::GVFlags(Linkage, NoRename, - HasInlineAsmMaybeReferencingInternal, - IsNotViableToInline); + bool NotEligibleToImport = (RawFlags & 0x1) || Version < 3; + return GlobalValueSummary::GVFlags(Linkage, NotEligibleToImport); } static GlobalValue::VisibilityTypes getDecodedVisibility(unsigned Val) { @@ -4838,9 +4834,9 @@ } const uint64_t Version = Record[0]; const bool IsOldProfileFormat = Version == 1; - if (!IsOldProfileFormat && Version != 2) + if (Version < 1 || Version > 3) return error("Invalid summary version " + Twine(Version) + - ", 1 or 2 expected"); + ", 1, 2 or 3 expected"); Record.clear(); // Keep around the last seen summary to be used when we see an optional Index: llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp =================================================================== --- llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp +++ llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp @@ -971,9 +971,7 @@ static uint64_t getEncodedGVSummaryFlags(GlobalValueSummary::GVFlags Flags) { uint64_t RawFlags = 0; - RawFlags |= Flags.NoRename; // bool - RawFlags |= (Flags.IsNotViableToInline << 1); - RawFlags |= (Flags.HasInlineAsmMaybeReferencingInternal << 2); + RawFlags |= Flags.NotEligibleToImport; // bool // Linkage don't need to be remapped at that time for the summary. Any future // change to the getEncodedLinkage() function will need to be taken into // account here as well. @@ -3435,7 +3433,7 @@ // Current version for the summary. // This is bumped whenever we introduce changes in the way some record are // interpreted, like flags for instance. -static const uint64_t INDEX_VERSION = 2; +static const uint64_t INDEX_VERSION = 3; /// Emit the per-module summary section alongside the rest of /// the module's bitcode. Index: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp =================================================================== --- llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp +++ llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp @@ -105,78 +105,6 @@ namespace { -// Return true if the Summary describes a GlobalValue that can be externally -// referenced, i.e. it does not need renaming (linkage is not local) or renaming -// is possible (does not have a section for instance). -static bool canBeExternallyReferenced(const GlobalValueSummary &Summary) { - if (!Summary.needsRenaming()) - return true; - - if (Summary.noRename()) - // Can't externally reference a global that needs renaming if has a section - // or is referenced from inline assembly, for example. - return false; - - return true; -} - -// Return true if \p GUID describes a GlobalValue that can be externally -// referenced, i.e. it does not need renaming (linkage is not local) or -// renaming is possible (does not have a section for instance). -static bool canBeExternallyReferenced(const ModuleSummaryIndex &Index, - GlobalValue::GUID GUID) { - auto Summaries = Index.findGlobalValueSummaryList(GUID); - if (Summaries == Index.end()) - return true; - if (Summaries->second.size() != 1) - // If there are multiple globals with this GUID, then we know it is - // not a local symbol, and it is necessarily externally referenced. - return true; - - // We don't need to check for the module path, because if it can't be - // externally referenced and we call it, it is necessarilly in the same - // module - return canBeExternallyReferenced(**Summaries->second.begin()); -} - -// Return true if the global described by \p Summary can be imported in another -// module. -static bool eligibleForImport(const ModuleSummaryIndex &Index, - const GlobalValueSummary &Summary) { - if (!canBeExternallyReferenced(Summary)) - // Can't import a global that needs renaming if has a section for instance. - // FIXME: we may be able to import it by copying it without promotion. - return false; - - // Don't import functions that are not viable to inline. - if (Summary.isNotViableToInline()) - return false; - - // Check references (and potential calls) in the same module. If the current - // value references a global that can't be externally referenced it is not - // eligible for import. First check the flag set when we have possible - // opaque references (e.g. inline asm calls), then check the call and - // reference sets. - if (Summary.hasInlineAsmMaybeReferencingInternal()) - return false; - bool AllRefsCanBeExternallyReferenced = - llvm::all_of(Summary.refs(), [&](const ValueInfo &VI) { - return canBeExternallyReferenced(Index, VI.getGUID()); - }); - if (!AllRefsCanBeExternallyReferenced) - return false; - - if (auto *FuncSummary = dyn_cast(&Summary)) { - bool AllCallsCanBeExternallyReferenced = llvm::all_of( - FuncSummary->calls(), [&](const FunctionSummary::EdgeTy &Edge) { - return canBeExternallyReferenced(Index, Edge.first.getGUID()); - }); - if (!AllCallsCanBeExternallyReferenced) - return false; - } - return true; -} - /// Given a list of possible callee implementation for a call site, select one /// that fits the \p Threshold. /// @@ -214,7 +142,7 @@ if (Summary->instCount() > Threshold) return false; - if (!eligibleForImport(Index, *Summary)) + if (Summary->notEligibleToImport()) return false; return true; Index: llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp =================================================================== --- llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp +++ llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp @@ -56,12 +56,10 @@ if (!isPerformingImport() && !isModuleExporting()) return false; - // If we are exporting, we need to see whether this value is marked - // as NoRename in the summary. If we are importing, we may not have - // a summary in the distributed backend case (only summaries for values - // importes as defs, not references, are included in the index passed - // to the distributed backends). if (isPerformingImport()) { + assert(!GlobalsToImport->count(SGV) || + !isNonRenamableLocal(*SGV) && + "Attempting to promote non-renamable local"); // We don't know for sure yet if we are importing this value (as either // a reference or a def), since we are simply walking all values in the // module. But by necessity if we end up importing it and it is local, @@ -77,13 +75,28 @@ assert(Summaries->second.size() == 1 && "Local has more than one summary"); auto Linkage = Summaries->second.front()->linkage(); if (!GlobalValue::isLocalLinkage(Linkage)) { - assert(!Summaries->second.front()->noRename()); + assert(!isNonRenamableLocal(*SGV) && + "Attempting to promote non-renamable local"); return true; } return false; } +#ifndef NDEBUG +bool FunctionImportGlobalProcessing::isNonRenamableLocal( + const GlobalValue &GV) const { + if (!GV.hasLocalLinkage()) + return false; + // This needs to stay in sync with the logic in buildModuleSummaryIndex. + if (GV.hasSection()) + return true; + if (Used.count(const_cast(&GV))) + return true; + return false; +} +#endif + std::string FunctionImportGlobalProcessing::getName(const GlobalValue *SGV, bool DoPromote) { // For locals that must be promoted to global scope, ensure that Index: llvm/trunk/test/Bitcode/summary_version.ll =================================================================== --- llvm/trunk/test/Bitcode/summary_version.ll +++ llvm/trunk/test/Bitcode/summary_version.ll @@ -2,7 +2,7 @@ ; RUN: opt -module-summary %s -o - | llvm-bcanalyzer -dump | FileCheck %s ; CHECK: +; CHECK: Index: llvm/trunk/test/Bitcode/thinlto-function-summary.ll =================================================================== --- llvm/trunk/test/Bitcode/thinlto-function-summary.ll +++ llvm/trunk/test/Bitcode/thinlto-function-summary.ll @@ -10,7 +10,7 @@ ; BC-NEXT: