[PATCH] D18343: ThinLTO: use the callgraph from the combined index to drive the FunctionImporter

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:70
@@ +69,3 @@
+/// FIXME: select "best" instead of first that fits. But what is "best"? The
+/// smallest? The one with the least edges? A mix?
+static const FunctionSummary *
----------------
davidxl wrote:
> tejohnson wrote:
> > - Prioritize copies from a module already being imported from (reduces # source modules parsed/linked)
> > - Prioritize any that has profile data, although we don't have that info here (not sure how COMDAT profile matching works on llvm, I know on gcc only the selected or any inlined/selected copies would get profile data, may be different on llvm due to single profile database?)
> In LLVM, comdat function's profile counters will also be put into comdat, so there should be no need to differentiate based on profile availability.
Good news!

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:422
@@ +421,3 @@
+    StringMap<std::unordered_set<uint64_t>> ExportLists;
+    ComputeCrossModuleImport(*Index, ImportLists, ExportLists);
+    auto &ImportList = ImportLists[M.getModuleIdentifier()];
----------------
joker.eph wrote:
> tejohnson wrote:
> > Note the export lists are not useful at this point since in this case we are already in the back end and we can't communicate this info to the source module which is presumably being compiled in a separate thread or backend process (in the distributed build case). Probably needs a comment about needing to be conservative when function importing being done in the backend and assume for now that all symbols may be exported. To get better about this in the distributed build case we will need to compute this info in the plugin and save it somewhere (in the index?) for consumption by the backends. For the single machine case where gold is the linker it will have to be reworked similar to what you have done in LTOCodeGenerator to do the importing computation ahead of time.
> You can call `ComputeCrossModuleImport` and get the same answer in every backend jobs.
> By moving this code a few lines above before the `renameModuleforThinLTO` we'll be able to use the export list to limit the renaming in the source module.
As discussed on IRC, this will work for the time being (assuming all invocations use the same thresholds etc). Longer term, compute this info in the plugin (and serialize out for distributed build backends).


http://reviews.llvm.org/D18343