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

[Teresa Johnson via llvm-commits]

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?

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

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

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.


http://reviews.llvm.org/D15102