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

[Teresa Johnson via llvm-commits]

On Tue, Dec 1, 2015 at 8:25 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:
>
>> 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?

I think we will want to be more flexible in the importing strategy and
not force all the decisions to be made a priori.

Teresa

>
>> 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
>



-- 
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413