[llvm] r254486 - Change ModuleLinker to take a set of GlobalValues to import instead of a single one
Mehdi Amini via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 2 18:45:21 PST 2015
> On Dec 2, 2015, at 3:02 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
>
> On Tue, Dec 1, 2015 at 8:34 PM, Mehdi Amini via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
> Author: mehdi_amini
> Date: Tue Dec 1 22:34:28 2015
> New Revision: 254486
>
> URL: http://llvm.org/viewvc/llvm-project?rev=254486&view=rev <http://llvm.org/viewvc/llvm-project?rev=254486&view=rev>
> Log:
> Change ModuleLinker to take a set of GlobalValues to import instead of a single one
>
> For efficiency reason, when importing multiple functions for the same Module,
> we can avoid reparsing it every time.
>
> Differential Revision: http://reviews.llvm.org/D15102 <http://reviews.llvm.org/D15102>
>
> From: Mehdi Amini <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>>
>
> Modified:
> llvm/trunk/include/llvm/Linker/Linker.h
> llvm/trunk/lib/Linker/LinkModules.cpp
> llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
> llvm/trunk/tools/llvm-link/llvm-link.cpp
>
> Modified: llvm/trunk/include/llvm/Linker/Linker.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Linker/Linker.h?rev=254486&r1=254485&r2=254486&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Linker/Linker.h?rev=254486&r1=254485&r2=254486&view=diff>
> ==============================================================================
> --- llvm/trunk/include/llvm/Linker/Linker.h (original)
> +++ llvm/trunk/include/llvm/Linker/Linker.h Tue Dec 1 22:34:28 2015
> @@ -82,7 +82,7 @@ public:
> /// Returns true on error.
> bool linkInModule(Module &Src, unsigned Flags = Flags::None,
> const FunctionInfoIndex *Index = nullptr,
> - Function *FuncToImport = nullptr);
> + DenseSet<const GlobalValue *> *FuncToImport = nullptr);
>
> static bool linkModules(Module &Dest, Module &Src,
> DiagnosticHandlerFunction DiagnosticHandler,
>
> Modified: llvm/trunk/lib/Linker/LinkModules.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/LinkModules.cpp?rev=254486&r1=254485&r2=254486&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/LinkModules.cpp?rev=254486&r1=254485&r2=254486&view=diff>
> ==============================================================================
> --- llvm/trunk/lib/Linker/LinkModules.cpp (original)
> +++ llvm/trunk/lib/Linker/LinkModules.cpp Tue Dec 1 22:34:28 2015
> @@ -406,7 +406,7 @@ class ModuleLinker {
>
> /// Function to import from source module, all other functions are
> /// imported as declarations instead of definitions.
> - Function *ImportFunction;
> + DenseSet<const GlobalValue *> *ImportFunction;
>
> /// Set to true if the given FunctionInfoIndex contains any functions
> /// from this source module, in which case we must conservatively assume
> @@ -425,7 +425,7 @@ public:
> ModuleLinker(Module &DstM, Linker::IdentifiedStructTypeSet &Set, Module &SrcM,
> DiagnosticHandlerFunction DiagnosticHandler, unsigned Flags,
> const FunctionInfoIndex *Index = nullptr,
> - Function *FuncToImport = nullptr)
> + DenseSet<const GlobalValue *> *FuncToImport = nullptr)
> : DstM(DstM), SrcM(SrcM), TypeMap(Set), ValMaterializer(this),
> DiagnosticHandler(DiagnosticHandler), Flags(Flags), ImportIndex(Index),
> ImportFunction(FuncToImport) {
> @@ -632,7 +632,7 @@ bool ModuleLinker::doImportAsDefinition(
> return true;
> // Only import the function requested for importing.
> auto *SF = dyn_cast<Function>(SGV);
> - if (SF && SF == ImportFunction)
> + if (SF && ImportFunction->count(SF))
> return true;
> // Otherwise no.
> return false;
> @@ -1058,7 +1058,7 @@ bool ModuleLinker::shouldLinkFromSource(
> if (isa<Function>(&Src)) {
> // For functions, LinkFromSrc iff this is the function requested
> // for importing. For variables, decide below normally.
> - LinkFromSrc = (&Src == ImportFunction);
> + LinkFromSrc = ImportFunction->count(&Src);
> return false;
> }
>
> @@ -2033,7 +2033,7 @@ Linker::Linker(Module &M)
>
> bool Linker::linkInModule(Module &Src, unsigned Flags,
> const FunctionInfoIndex *Index,
> - Function *FuncToImport) {
> + DenseSet<const GlobalValue *> *FuncToImport) {
> ModuleLinker TheLinker(Composite, IdentifiedStructTypes, Src,
> DiagnosticHandler, Flags, Index, FuncToImport);
> bool RetCode = TheLinker.run();
>
> Modified: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp?rev=254486&r1=254485&r2=254486&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp?rev=254486&r1=254485&r2=254486&view=diff>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp (original)
> +++ llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp Tue Dec 1 22:34:28 2015
> @@ -182,7 +182,10 @@ bool FunctionImporter::importFunctions(M
> }
>
> // Link in the specified function.
> - if (L.linkInModule(Module, Linker::Flags::None, &Index, F))
> + DenseSet<const GlobalValue *> FunctionsToImport;
> + FunctionsToImport.insert(F);
> + if (L.linkInModule(Module, Linker::Flags::None, &Index,
> + &FunctionsToImport))
> report_fatal_error("Function Import: link error");
>
> // Process the newly imported function and add callees to the worklist.
> @@ -194,6 +197,7 @@ bool FunctionImporter::importFunctions(M
>
> Changed = true;
> }
> +
> return Changed;
> }
>
>
> Modified: llvm/trunk/tools/llvm-link/llvm-link.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-link/llvm-link.cpp?rev=254486&r1=254485&r2=254486&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-link/llvm-link.cpp?rev=254486&r1=254485&r2=254486&view=diff>
> ==============================================================================
> --- llvm/trunk/tools/llvm-link/llvm-link.cpp (original)
> +++ llvm/trunk/tools/llvm-link/llvm-link.cpp Tue Dec 1 22:34:28 2015
> @@ -198,7 +198,10 @@ static bool importFunctions(const char *
> }
>
> // Link in the specified function.
> - if (L.linkInModule(*M, Linker::Flags::None, Index.get(), F))
> + DenseSet<const GlobalValue *> FunctionToImport;
> + FunctionToImport.insert(F);
> + if (L.linkInModule(*M, Linker::Flags::None, Index.get(),
> + &FunctionToImport))
>
> 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.
> It is also inconsistent with FunctionImport.cpp where you do use the name "FunctionsToImport”.
Good point, fixed in r254582/r254584. It was even looking worse in the ModuleLinker.
Thanks!
—
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151202/fcc4cdc7/attachment.html>
More information about the llvm-commits
mailing list