[llvm] r255100 - The current importing scheme is processing one function at a time,
Alexey Samsonov via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 9 12:10:19 PST 2015
On Wed, Dec 9, 2015 at 12:17 AM, Mehdi Amini via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: mehdi_amini
> Date: Wed Dec 9 02:17:35 2015
> New Revision: 255100
>
> URL: http://llvm.org/viewvc/llvm-project?rev=255100&view=rev
> Log:
> The current importing scheme is processing one function at a time,
> loading the source Module, linking the function in the destination
> module, and destroying the source Module before repeating with the
> next function to import (potentially from the same Module).
>
> Ideally we would keep the source Module alive and import the next
> Function needed from this Module. Unfortunately this is not possible
> because the linker does not leave it in a usable state.
>
> However we can do better by first computing the list of all candidates
> per Module, and only then load the source Module and import all the
> function we need for it.
>
> The trick to process callees is to materialize function in the source
> module when building the list of function to import, and inspect them
> in their source module, collecting the list of callees for each
> callee.
>
> When we move the the actual import, we will import from each source
> module exactly once. Each source module is loaded exactly once.
> The only drawback it that it requires to have all the lazy-loaded
> source Module in memory at the same time.
>
> Currently this patch already improves considerably the link time,
> a multithreaded link of llvm-dis on my laptop was:
>
> real 1m12.175s user 6m32.430s sys 0m10.529s
>
> and is now:
>
> real 0m40.697s user 2m10.237s sys 0m4.375s
>
> Note: this is the full link time (linker+Import+Optimizer+CodeGen)
>
> Differential Revision: http://reviews.llvm.org/D15178
>
> From: Mehdi Amini <mehdi.amini at apple.com>
>
> Modified:
> llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
>
> Modified: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp?rev=255100&r1=255099&r2=255100&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp (original)
> +++ llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp Wed Dec 9 02:17:35
> 2015
> @@ -24,6 +24,9 @@
> #include "llvm/Support/CommandLine.h"
> #include "llvm/Support/Debug.h"
> #include "llvm/Support/SourceMgr.h"
> +
> +#include <map>
> +
> using namespace llvm;
>
> #define DEBUG_TYPE "function-import"
> @@ -50,37 +53,108 @@ static std::unique_ptr<Module> loadFile(
> return Result;
> }
>
> +namespace {
> +/// Helper to load on demand a Module from file and cache it for
> subsequent
> +/// queries. It can be used with the FunctionImporter.
> +class ModuleLazyLoaderCache {
> + /// Cache of lazily loaded module for import.
> + StringMap<std::unique_ptr<Module>> ModuleMap;
> +
> + /// Retrieve a Module from the cache or lazily load it on demand.
> + std::function<std::unique_ptr<Module>(StringRef FileName)>
> createLazyModule;
> +
> +public:
> + /// Create the loader, Module will be initialized in \p Context.
> + ModuleLazyLoaderCache(std::function<
> + std::unique_ptr<Module>(StringRef FileName)> createLazyModule)
> + : createLazyModule(createLazyModule) {}
> +
> + /// Retrieve a Module from the cache or lazily load it on demand.
> + Module &operator()(StringRef FileName);
> +};
> +
> +// Get a Module for \p FileName from the cache, or load it lazily.
> +Module &ModuleLazyLoaderCache::operator()(StringRef Identifier) {
> + auto &Module = ModuleMap[Identifier];
> + if (!Module)
> + Module = createLazyModule(Identifier);
> + return *Module;
> +}
> +} // anonymous namespace
> +
> /// Walk through the instructions in \p F looking for external
> /// calls not already in the \p CalledFunctions set. If any are
> /// found they are added to the \p Worklist for importing.
> -static void findExternalCalls(const Function &F, StringSet<>
> &CalledFunctions,
> +static void findExternalCalls(const Module &DestModule, Function &F,
> + const FunctionInfoIndex &Index,
> + StringSet<> &CalledFunctions,
> SmallVector<StringRef, 64> &Worklist) {
> + // We need to suffix internal function calls imported from other
> modules,
> + // prepare the suffix ahead of time.
> + StringRef Suffix;
> + if (F.getParent() != &DestModule)
> + Suffix =
> + (Twine(".llvm.") +
> +
> Twine(Index.getModuleId(F.getParent()->getModuleIdentifier()))).str();
>
^^
No. Please don't assign temporary string to StringRef. This was detected by
one of our ASan bootstrap bots.
> +
> for (auto &BB : F) {
> for (auto &I : BB) {
> if (isa<CallInst>(I)) {
> auto CalledFunction = cast<CallInst>(I).getCalledFunction();
> // Insert any new external calls that have not already been
> // added to set/worklist.
> - if (CalledFunction && CalledFunction->hasName() &&
> - CalledFunction->isDeclaration() &&
> - !CalledFunctions.count(CalledFunction->getName())) {
> - CalledFunctions.insert(CalledFunction->getName());
> - Worklist.push_back(CalledFunction->getName());
> + if (!CalledFunction || !CalledFunction->hasName())
> + continue;
> + // Ignore intrinsics early
> + if (CalledFunction->isIntrinsic()) {
> + assert(CalledFunction->getIntrinsicID() != 0);
> + continue;
> + }
> + auto ImportedName = CalledFunction->getName();
> + auto Renamed = (ImportedName + Suffix).str();
> + // Rename internal functions
> + if (CalledFunction->hasInternalLinkage()) {
> + ImportedName = Renamed;
> + }
> + auto It = CalledFunctions.insert(ImportedName);
> + if (!It.second) {
> + // This is a call to a function we already considered, skip.
> + continue;
> }
> + // Ignore functions already present in the destination module
> + auto *SrcGV = DestModule.getNamedValue(ImportedName);
> + if (SrcGV) {
> + assert(isa<Function>(SrcGV) && "Name collision during import");
> + if (!cast<Function>(SrcGV)->isDeclaration()) {
> + DEBUG(dbgs() << DestModule.getModuleIdentifier() << "Ignoring
> "
> + << ImportedName << " already in
> DestinationModule\n");
> + continue;
> + }
> + }
> +
> + Worklist.push_back(It.first->getKey());
> + DEBUG(dbgs() << DestModule.getModuleIdentifier()
> + << " Adding callee for : " << ImportedName << " : "
> + << F.getName() << "\n");
> }
> }
> }
> }
>
> // Helper function: given a worklist and an index, will process all the
> worklist
> -// and import them based on the summary information
> -static unsigned ProcessImportWorklist(
> +// and decide what to import based on the summary information.
> +//
> +// Nothing is actually imported, functions are materialized in their
> source
> +// module and analyzed there.
> +//
> +// \p ModuleToFunctionsToImportMap is filled with the set of Function to
> import
> +// per Module.
> +static void GetImportList(
> Module &DestModule, SmallVector<StringRef, 64> &Worklist,
> - StringSet<> &CalledFunctions, Linker &TheLinker,
> - const FunctionInfoIndex &Index,
> - std::function<std::unique_ptr<Module>(StringRef FileName)> &
> - LazyModuleLoader) {
> - unsigned ImportCount = 0;
> + StringSet<> &CalledFunctions,
> + std::map<StringRef, std::pair<Module *, DenseSet<const GlobalValue
> *>>> &
> + ModuleToFunctionsToImportMap,
> + const FunctionInfoIndex &Index, ModuleLazyLoaderCache
> &ModuleLoaderCache) {
> while (!Worklist.empty()) {
> auto CalledFunctionName = Worklist.pop_back_val();
> DEBUG(dbgs() << DestModule.getModuleIdentifier() << "Process import
> for "
> @@ -116,40 +190,39 @@ static unsigned ProcessImportWorklist(
> }
>
> // Get the module path from the summary.
> - auto FileName = Summary->modulePath();
> - DEBUG(dbgs() << "Importing " << CalledFunctionName << " from " <<
> FileName
> - << "\n");
> + auto ModuleIdentifier = Summary->modulePath();
> + DEBUG(dbgs() << DestModule.getModuleIdentifier() << " Importing "
> + << CalledFunctionName << " from " << ModuleIdentifier <<
> "\n");
>
> - // Get the module for the import
> - auto SrcModule = LazyModuleLoader(FileName);
> - assert(&SrcModule->getContext() == &DestModule.getContext());
> + auto &SrcModule = ModuleLoaderCache(ModuleIdentifier);
>
> // The function that we will import!
> - GlobalValue *SGV = SrcModule->getNamedValue(CalledFunctionName);
> - StringRef ImportFunctionName = CalledFunctionName;
> + GlobalValue *SGV = SrcModule.getNamedValue(CalledFunctionName);
> +
> if (!SGV) {
> - // Might be local in source Module, promoted/renamed in DestModule.
> + // The destination module is referencing function using their
> renamed name
> + // when importing a function that was originally local in the source
> + // module. The source module we have might not have been renamed so
> we try
> + // to remove the suffix added during the renaming to recover the
> original
> + // name in the source module.
> std::pair<StringRef, StringRef> Split =
> CalledFunctionName.split(".llvm.");
> - SGV = SrcModule->getNamedValue(Split.first);
> -#ifndef NDEBUG
> - // Assert that Split.second is module id
> - uint64_t ModuleId;
> - assert(!Split.second.getAsInteger(10, ModuleId));
> - assert(ModuleId == Index.getModuleId(FileName));
> -#endif
> + SGV = SrcModule.getNamedValue(Split.first);
> + assert(SGV && "Can't find function to import in source module");
> + }
> + if (!SGV) {
> + report_fatal_error(Twine("Can't load function '") +
> CalledFunctionName +
> + "' in Module '" +
> SrcModule.getModuleIdentifier() +
> + "', error in the summary?\n");
> }
> +
> Function *F = dyn_cast<Function>(SGV);
> if (!F && isa<GlobalAlias>(SGV)) {
> auto *SGA = dyn_cast<GlobalAlias>(SGV);
> F = dyn_cast<Function>(SGA->getBaseObject());
> - ImportFunctionName = F->getName();
> - }
> - if (!F) {
> - errs() << "Can't load function '" << CalledFunctionName << "' in
> Module '"
> - << FileName << "', error in the summary?\n";
> - llvm_unreachable("Can't load function in Module");
> + CalledFunctionName = F->getName();
> }
> + assert(F && "Imported Function is ... not a Function");
>
> // We cannot import weak_any functions/aliases without possibly
> affecting
> // the order they are seen and selected by the linker, changing
> program
> @@ -158,26 +231,20 @@ static unsigned ProcessImportWorklist(
> DEBUG(dbgs() << DestModule.getModuleIdentifier()
> << " Ignoring import request for weak-any "
> << (isa<Function>(SGV) ? "function " : "alias ")
> - << CalledFunctionName << " from " << FileName << "\n");
> + << CalledFunctionName << " from "
> + << SrcModule.getModuleIdentifier() << "\n");
> continue;
> }
>
> - // Link in the specified function.
> - DenseSet<const GlobalValue *> FunctionsToImport;
> - FunctionsToImport.insert(F);
> - if (TheLinker.linkInModule(*SrcModule, Linker::Flags::None, &Index,
> - &FunctionsToImport))
> - report_fatal_error("Function Import: link error");
> -
> - // Process the newly imported function and add callees to the
> worklist.
> - GlobalValue *NewGV = DestModule.getNamedValue(ImportFunctionName);
> - assert(NewGV);
> - Function *NewF = dyn_cast<Function>(NewGV);
> - assert(NewF);
> - findExternalCalls(*NewF, CalledFunctions, Worklist);
> - ++ImportCount;
> + // Add the function to the import list
> + auto &Entry =
> ModuleToFunctionsToImportMap[SrcModule.getModuleIdentifier()];
> + Entry.first = &SrcModule;
> + Entry.second.insert(F);
> +
> + // Process the newly imported functions and add callees to the
> worklist.
> + F->materialize();
> + findExternalCalls(DestModule, *F, Index, CalledFunctions, Worklist);
> }
> - return ImportCount;
> }
>
> // Automatically import functions in Module \p DestModule based on the
> summaries
> @@ -196,7 +263,7 @@ bool FunctionImporter::importFunctions(M
> for (auto &F : DestModule) {
> if (F.isDeclaration() || F.hasFnAttribute(Attribute::OptimizeNone))
> continue;
> - findExternalCalls(F, CalledFunctions, Worklist);
> + findExternalCalls(DestModule, F, Index, CalledFunctions, Worklist);
> }
> if (Worklist.empty())
> return false;
> @@ -206,10 +273,33 @@ bool FunctionImporter::importFunctions(M
> // Linker that will be used for importing function
> Linker TheLinker(DestModule, DiagnosticHandler);
>
> - ImportedCount += ProcessImportWorklist(DestModule, Worklist,
> CalledFunctions,
> - TheLinker, Index, ModuleLoader);
> + // Map of Module -> List of Function to import from the Module
> + std::map<StringRef, std::pair<Module *, DenseSet<const GlobalValue *>>>
> + ModuleToFunctionsToImportMap;
> +
> + // Analyze the summaries and get the list of functions to import by
> + // populating ModuleToFunctionsToImportMap
> + ModuleLazyLoaderCache ModuleLoaderCache(ModuleLoader);
> + GetImportList(DestModule, Worklist, CalledFunctions,
> + ModuleToFunctionsToImportMap, Index, ModuleLoaderCache);
> + assert(Worklist.empty() && "Worklist hasn't been flushed in
> GetImportList");
>
> - DEBUG(errs() << "Imported " << ImportedCount << " functions for Module "
> + // Do the actual import of functions now, one Module at a time
> + for (auto &FunctionsToImportPerModule : ModuleToFunctionsToImportMap) {
> + // Get the module for the import
> + auto &FunctionsToImport = FunctionsToImportPerModule.second.second;
> + auto *SrcModule = FunctionsToImportPerModule.second.first;
> + assert(&DestModule.getContext() == &SrcModule->getContext() &&
> + "Context mismatch");
> +
> + // Link in the specified functions.
> + if (TheLinker.linkInModule(*SrcModule, Linker::Flags::None, &Index,
> + &FunctionsToImport))
> + report_fatal_error("Function Import: link error");
> +
> + ImportedCount += FunctionsToImport.size();
> + }
> + DEBUG(dbgs() << "Imported " << ImportedCount << " functions for Module "
> << DestModule.getModuleIdentifier() << "\n");
> return ImportedCount;
> }
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
--
Alexey Samsonov
vonosmas at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151209/d1a5c359/attachment.html>
More information about the llvm-commits
mailing list