[PATCH] D15102: Change ModuleLinker to take a set of GlobalValues to import instead of a single one

[Mehdi Amini via llvm-commits]

> On Dec 1, 2015, at 6:49 AM, Teresa Johnson <tejohnson at google.com> wrote:
> 
> tejohnson added inline comments.
> 
> ================
> Comment at: lib/Transforms/IPO/FunctionImport.cpp:85
> @@ -87,1 +84,3 @@
> +  DenseMap<Module *, DenseSet<const GlobalValue *>>
> +      ModuleToFunctionsToImportMap;
> 
> ----------------
> I don't see this map being populated anywhere?

Looks like I was eager to go to bed yesterday night. Thanks for catching.

> 
> ================
> Comment at: lib/Transforms/IPO/FunctionImport.cpp:153
> @@ -153,4 +152,3 @@
> 
> -    // Link in the specified function.
> -    if (L.linkInModule(&Module, Linker::Flags::None, &Index, F))
> -      report_fatal_error("Function Import: link error");
> +    // TODO: add callees from the newly imported function to the worklist.
> +  }
> ----------------
> Needs merge - I have already implemented this functionality in head.


I knew I was forgetting something when coming back from the break ;)

I’ll have a look how to update.


> 
> ================
> Comment at: lib/Transforms/IPO/FunctionImport.cpp:161
> @@ +160,3 @@
> +
> +  for (auto It : ModuleToFunctionsToImportMap) {
> +    auto *Module = It.first;
> ----------------
> This fundamentally changes the pass from doing an iterative worklist approach to a single pass of import per module, and makes it more difficult to go to a priority queue type approach. I don't think it will merge with the changes I made from head for adding newly imported external calls to the worklist actually.
> 
> We may not decide it is profitable to import every external call into a particular module at once. E.g. we may see a cold-ish call into an external module, and later after another import expose a hotter call that bumps its priority. I'm fine with passing in a list of functions to import in one go though - there may be multiple calls into that module that have similar priority, and they can be batch imported. 

Yeah, all of this is very annoying :(
What about adding the callees to the summary so that we can compute everything ahead and then bulk import?

> Perhaps add in the mechanics for doing a list of imports, but leave out the changes to this function for now (and just continue to pass in a single function) until we figure out how that should work.

OK, will do.


Mehdi