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

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

Thanks, I'll fix all of these comments, a few answers in the meantime.


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:374
@@ +373,3 @@
+  StringMap<StringMap<std::map<uint64_t, unsigned>>> ImportLists;
+  StringMap<std::unordered_set<uint64_t>> ExportLists;
+  ComputeCrossModuleImport(*Index, ImportLists, ExportLists);
----------------
tejohnson wrote:
> Initialize these two maps with the module count as done in ThinLTOCodeGenerator::crossModuleImport
Yes! I was waiting for my changes on the StringMap to be checked in (committed yesterday).

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:352
@@ -378,3 +351,3 @@
 
-/// Parse the summary index out of an IR file and return the summary
+/// Parse the function index out of an IR file and return the function
 /// index object if found, or nullptr if not.
----------------
tejohnson wrote:
> This patch seems to undo the renaming done in r263513.
Probably an issue when rebasing on top of your changes.

================
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()];
----------------
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.

================
Comment at: test/Transforms/FunctionImport/adjustable_threshold.ll:7
@@ -6,3 +6,3 @@
 ; Test import with default progressive instruction factor
-; RUN: opt -function-import -summary-file %t3.thinlto.bc %s -import-instr-limit=10 -S | FileCheck %s --check-prefix=CHECK --check-prefix=INSTLIM-DEFAULT
+; RUN: opt -function-import -summary-file %t3.thinlto.bc %t.bc -import-instr-limit=10 -S | FileCheck %s --check-prefix=CHECK --check-prefix=INSTLIM-DEFAULT
 ; INSTLIM-DEFAULT: call void @staticfunc2.llvm.2()
----------------
tejohnson wrote:
> Why these changes of using .bc instead of .ll? Is it needed or at least independent and can be committed separately?
It is needed: the summary will contains the module path as a string pointing to the .bc.
The importer will not be able to walk the graph because if won't find the modules in the index.


http://reviews.llvm.org/D18343