[PATCH] D15178: FunctionImporter: implement bulk function importing for efficiency

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Thanks for working on this. Some more comments/questions below.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:86
@@ +85,3 @@
+//// Get a Module for \p FileName from the cache, or load it lazily.
+// Module &ModuleLazyLoaderCache::operator()(StringRef FileName) {
+//  auto &Module = ModuleMap[FileName];
----------------
Remove

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:100
@@ -65,1 +99,3 @@
                               SmallVector<StringRef, 64> &Worklist) {
+  // We need to suffix internal function imported from other modules, prepare
+  // the suffix ahead of time.
----------------
Maybe "...internal function calls imported from..."? I.e. at this point we have only imported the call. 

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:128
@@ +127,3 @@
+        if (!It.second) {
+          DEBUG(dbgs() << DestModule.getModuleIdentifier() << " Ignoring "
+                       << ImportedName << "\n");
----------------
We reach here if the function is already in the CalledFunctions set, so do we want to issue a message that we are ignoring it? It seems misleading, i.e. it sounds like we decided not to import.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:140
@@ +139,3 @@
+                         << ImportedName << " already in DestinationModule\n");
+            continue;
+          }
----------------
Why is this under #ifndef NDEBUG?

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:279
@@ -212,3 +278,3 @@
 
-  ImportedCount += ProcessImportWorklist(DestModule, Worklist, CalledFunctions,
-                                         TheLinker, Index, getLazyModule);
+  /// For each iteration, ProcessImportWorklist() will empty the Worklist and
+  /// then fill it with the callees of the newly imported functions, that we'll
----------------
I don't see anything called ProcessImportWorklist? Do you mean GetImportList? Each iteration of what?

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:283
@@ +282,3 @@
+  /// This workflow is there for efficiency, since batch-importing functions
+  /// in ProcessImportWorklist helps required by the ModuleLinker.
+
----------------
Comment reads funny: "helps required by"


http://reviews.llvm.org/D15178