[llvm] r255100 - The current importing scheme is processing one function at a time,
Rafael Espíndola via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 9 12:44:49 PST 2015
Should be fixed with r255149. We hit that with a windows debug build too.
Cheers,
Rafael
On 9 December 2015 at 15:10, Alexey Samsonov via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
> 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
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list