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