[PATCH] D18343: ThinLTO: use the callgraph from the combined index to drive the FunctionImporter
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 25 21:02:55 PDT 2016
tejohnson added inline comments.
Comment at: include/llvm/Transforms/IPO/FunctionImport.h:28
@@ +27,3 @@
+ /// 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/. 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;
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.
More information about the llvm-commits