[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: include/llvm/Transforms/IPO/FunctionImport.h:28
@@ +27,3 @@
+public:
+  /// List of function to import from a source module. each entry is a map
+  /// containing all the functions to import for a source module.
----------------
s/function/functions
s/. each/. Each/

================
Comment at: include/llvm/Transforms/IPO/FunctionImport.h:35
@@ +34,3 @@
+  /// The map contains an entry for every module to import from, the key being
+  /// the module identifier to pass to the ModuleLoader. The value for
+  typedef StringMap<FunctionsToImportTy> ImportMapTy;
----------------
Incomplete sentence "The value for"

================
Comment at: include/llvm/Transforms/IPO/FunctionImport.h:38
@@ -27,3 +37,3 @@
 
-  /// Factory function to load a Module for a given identifier
-  std::function<std::unique_ptr<Module>(StringRef Identifier)> ModuleLoader;
+  /// The map contains an entry for every functions the module exports.
+  typedef std::unordered_set<uint64_t> ExportMapTy;
----------------
s/functions/function/
Actually, this needs to hold global variables that are exported as well, right? Maybe say "for every global value the module exports".

Also, not a map. So maybe ExportSetTy in addition to comment fix?

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:157
@@ +156,3 @@
+    // as exported from outside as well.
+    // FIXME: we don't import any globals right now. We may want to import
+    // constants?
----------------
True that we don't import globals, but we do export references to them if they are referenced from a function that is imported elsewhere. So presumably those references should be added to the export list as they may need to be promoted/renamed.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:324
@@ +323,3 @@
+        // When the alias is pointing to a linkonceodr, don't import
+        // FIXME: why?
+        //          continue;
----------------
Oh, I know why. We can't have an alias to an available_externally def, since that is actually a declaration for the linker, and most imported defs are given available_externally linkage (except linkonce which stays the same linkage). This is the same logic as in FunctionImportGlobalProcessing::doImportAsDefinition(). I think there might have been more comments around that at one point but they gotten lost when the code moved around.


http://reviews.llvm.org/D18343