<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Dec 2, 2015, at 3:02 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com" class="">chisophugis@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class="Apple-interchange-newline"><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Tue, Dec 1, 2015 at 8:34 PM, Mehdi Amini via llvm-commits<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;">Author: mehdi_amini<br class="">Date: Tue Dec 1 22:34:28 2015<br class="">New Revision: 254486<br class=""><br class="">URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project?rev=254486&view=rev" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=254486&view=rev</a><br class="">Log:<br class="">Change ModuleLinker to take a set of GlobalValues to import instead of a single one<br class=""><br class="">For efficiency reason, when importing multiple functions for the same Module,<br class="">we can avoid reparsing it every time.<br class=""><br class="">Differential Revision:<span class="Apple-converted-space"> </span><a href="http://reviews.llvm.org/D15102" rel="noreferrer" target="_blank" class="">http://reviews.llvm.org/D15102</a><br class=""><br class="">From: Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" class="">mehdi.amini@apple.com</a>><br class=""><br class="">Modified:<br class=""> <span class="Apple-converted-space"> </span>llvm/trunk/include/llvm/Linker/Linker.h<br class=""> <span class="Apple-converted-space"> </span>llvm/trunk/lib/Linker/LinkModules.cpp<br class=""> <span class="Apple-converted-space"> </span>llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp<br class=""> <span class="Apple-converted-space"> </span>llvm/trunk/tools/llvm-link/llvm-link.cpp<br class=""><br class="">Modified: llvm/trunk/include/llvm/Linker/Linker.h<br class="">URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Linker/Linker.h?rev=254486&r1=254485&r2=254486&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Linker/Linker.h?rev=254486&r1=254485&r2=254486&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/include/llvm/Linker/Linker.h (original)<br class="">+++ llvm/trunk/include/llvm/Linker/Linker.h Tue Dec 1 22:34:28 2015<br class="">@@ -82,7 +82,7 @@ public:<br class=""> /// Returns true on error.<br class=""> bool linkInModule(Module &Src, unsigned Flags = Flags::None,<br class=""> const FunctionInfoIndex *Index = nullptr,<br class="">- Function *FuncToImport = nullptr);<br class="">+ DenseSet<const GlobalValue *> *FuncToImport = nullptr);<br class=""><br class=""> static bool linkModules(Module &Dest, Module &Src,<br class=""> DiagnosticHandlerFunction DiagnosticHandler,<br class=""><br class="">Modified: llvm/trunk/lib/Linker/LinkModules.cpp<br class="">URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/LinkModules.cpp?rev=254486&r1=254485&r2=254486&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/LinkModules.cpp?rev=254486&r1=254485&r2=254486&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/lib/Linker/LinkModules.cpp (original)<br class="">+++ llvm/trunk/lib/Linker/LinkModules.cpp Tue Dec 1 22:34:28 2015<br class="">@@ -406,7 +406,7 @@ class ModuleLinker {<br class=""><br class=""> /// Function to import from source module, all other functions are<br class=""> /// imported as declarations instead of definitions.<br class="">- Function *ImportFunction;<br class="">+ DenseSet<const GlobalValue *> *ImportFunction;<br class=""><br class=""> /// Set to true if the given FunctionInfoIndex contains any functions<br class=""> /// from this source module, in which case we must conservatively assume<br class="">@@ -425,7 +425,7 @@ public:<br class=""> ModuleLinker(Module &DstM, Linker::IdentifiedStructTypeSet &Set, Module &SrcM,<br class=""> <span class="Apple-converted-space"> </span>DiagnosticHandlerFunction DiagnosticHandler, unsigned Flags,<br class=""> <span class="Apple-converted-space"> </span>const FunctionInfoIndex *Index = nullptr,<br class="">- Function *FuncToImport = nullptr)<br class="">+ DenseSet<const GlobalValue *> *FuncToImport = nullptr)<br class=""> : DstM(DstM), SrcM(SrcM), TypeMap(Set), ValMaterializer(this),<br class=""> DiagnosticHandler(DiagnosticHandler), Flags(Flags), ImportIndex(Index),<br class=""> ImportFunction(FuncToImport) {<br class="">@@ -632,7 +632,7 @@ bool ModuleLinker::doImportAsDefinition(<br class=""> return true;<br class=""> // Only import the function requested for importing.<br class=""> auto *SF = dyn_cast<Function>(SGV);<br class="">- if (SF && SF == ImportFunction)<br class="">+ if (SF && ImportFunction->count(SF))<br class=""> return true;<br class=""> // Otherwise no.<br class=""> return false;<br class="">@@ -1058,7 +1058,7 @@ bool ModuleLinker::shouldLinkFromSource(<br class=""> if (isa<Function>(&Src)) {<br class=""> // For functions, LinkFromSrc iff this is the function requested<br class=""> // for importing. For variables, decide below normally.<br class="">- LinkFromSrc = (&Src == ImportFunction);<br class="">+ LinkFromSrc = ImportFunction->count(&Src);<br class=""> return false;<br class=""> }<br class=""><br class="">@@ -2033,7 +2033,7 @@ Linker::Linker(Module &M)<br class=""><br class=""> bool Linker::linkInModule(Module &Src, unsigned Flags,<br class=""> const FunctionInfoIndex *Index,<br class="">- Function *FuncToImport) {<br class="">+ DenseSet<const GlobalValue *> *FuncToImport) {<br class=""> ModuleLinker TheLinker(Composite, IdentifiedStructTypes, Src,<br class=""> <span class="Apple-converted-space"> </span>DiagnosticHandler, Flags, Index, FuncToImport);<br class=""> bool RetCode = TheLinker.run();<br class=""><br class="">Modified: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp<br class="">URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp?rev=254486&r1=254485&r2=254486&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp?rev=254486&r1=254485&r2=254486&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp (original)<br class="">+++ llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp Tue Dec 1 22:34:28 2015<br class="">@@ -182,7 +182,10 @@ bool FunctionImporter::importFunctions(M<br class=""> }<br class=""><br class=""> // Link in the specified function.<br class="">- if (L.linkInModule(Module, Linker::Flags::None, &Index, F))<br class="">+ DenseSet<const GlobalValue *> FunctionsToImport;<br class="">+ FunctionsToImport.insert(F);<br class="">+ if (L.linkInModule(Module, Linker::Flags::None, &Index,<br class="">+ &FunctionsToImport))<br class=""> report_fatal_error("Function Import: link error");<br class=""><br class=""> // Process the newly imported function and add callees to the worklist.<br class="">@@ -194,6 +197,7 @@ bool FunctionImporter::importFunctions(M<br class=""><br class=""> Changed = true;<br class=""> }<br class="">+<br class=""> return Changed;<br class=""> }<br class=""><br class=""><br class="">Modified: llvm/trunk/tools/llvm-link/llvm-link.cpp<br class="">URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-link/llvm-link.cpp?rev=254486&r1=254485&r2=254486&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-link/llvm-link.cpp?rev=254486&r1=254485&r2=254486&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/tools/llvm-link/llvm-link.cpp (original)<br class="">+++ llvm/trunk/tools/llvm-link/llvm-link.cpp Tue Dec 1 22:34:28 2015<br class="">@@ -198,7 +198,10 @@ static bool importFunctions(const char *<br class=""> }<br class=""><br class=""> // Link in the specified function.<br class="">- if (L.linkInModule(*M, Linker::Flags::None, Index.get(), F))<br class="">+ DenseSet<const GlobalValue *> FunctionToImport;<br class="">+ FunctionToImport.insert(F);<br class="">+ if (L.linkInModule(*M, Linker::Flags::None, Index.get(),<br class="">+ &FunctionToImport))<br class=""></blockquote><div class=""><br class=""></div><div class="">It's sort of weird to use "FunctionToImport" to describe a set. I would expect "FunctionsToImport", even if we know it will only contain one function.</div><div class="">It is also inconsistent with FunctionImport.cpp where you do use the name "FunctionsToImport”.</div></div></div></blockquote><div><br class=""></div><div><br class=""></div></div>Good point, fixed in r254582/r254584. It was even looking worse in the ModuleLinker.<div class=""><br class=""><div class="">Thanks!<div class=""><div class=""><br class=""></div><div class="">— </div><div class="">Mehdi</div><div class=""><br class=""></div></div></div></div></body></html>