[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 09:50:52 PDT 2016
tejohnson added a comment.
Thanks! A few comments/questions below.
================
Comment at: include/llvm/Transforms/IPO/FunctionImport.h:44
@@ -40,1 +43,3 @@
+ importFunctions(Module &M,
+ const StringMap<std::map<uint64_t, unsigned>> &ImportList);
};
----------------
Need a description of what is in each ImportList entry. Realize this could be deduced by reading ahead to the next comment, but would be good to include here too.
================
Comment at: include/llvm/Transforms/IPO/FunctionImport.h:49
@@ +48,3 @@
+///
+/// \p ImportLists will be populate with an enty for every Module. This entry is
+/// itself a map with an entry for each Module to import from, that contains a
----------------
s/enty/entry/
================
Comment at: include/llvm/Transforms/IPO/FunctionImport.h:50
@@ +49,3 @@
+/// \p ImportLists will be populate with an enty for every Module. This entry is
+/// itself a map with an entry for each Module to import from, that contains a
+/// map of imported function (GUID) and their imported Threshold.
----------------
Outer StringMap has an entry for every Module we are importing to, with an entry for each Module we are importing from, right? Maybe make that clearer by changing the first sentence above to "ImportLists will be populated with an entry for every Module we are importing into."
Might be clearer if you use typedefs for each map.
================
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);
----------------
Initialize these two maps with the module count as done in ThinLTOCodeGenerator::crossModuleImport
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:70
@@ +69,3 @@
+/// FIXME: select "best" instead of first that fits. But what is "best"? The
+/// smallest? The one with the least edges? A mix?
+static const FunctionSummary *
----------------
- Prioritize copies from a module already being imported from (reduces # source modules parsed/linked)
- Prioritize any that has profile data, although we don't have that info here (not sure how COMDAT profile matching works on llvm, I know on gcc only the selected or any inlined/selected copies would get profile data, may be different on llvm due to single profile database?)
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:75
@@ +74,3 @@
+ CalleeInfoList, [&](const std::unique_ptr<GlobalValueInfo> &GlobInfo) {
+ auto *Summary = dyn_cast_or_null<FunctionSummary>(GlobInfo->summary());
+ if (!Summary)
----------------
I think this can be an assert? We should have a summary for every GlobalValueInfo in the combined map, and presumably it should be a FunctionSummary type since we got here via the GUID of a call graph edge.
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:127
@@ -212,1 +126,3 @@
+ if (!CalleeSummary) {
+ DEBUG(dbgs() << "ignored!\n");
continue;
----------------
Give more meaningful message here? Maybe just as simple as "ignored! No qualifying callee with summary found."
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:130
@@ -213,14 +129,3 @@
}
- assert(!InfoList->second.empty() && "No summary, error at import?");
-
- // Comdat can have multiple entries, FIXME: what do we do with them?
- auto &Info = InfoList->second[0];
- assert(Info && "Nullptr in list, error importing summaries?\n");
-
- auto *Summary = dyn_cast<FunctionSummary>(Info->summary());
- if (!Summary) {
- // FIXME: in case we are lazyloading summaries, we can do it now.
- DEBUG(dbgs() << DestModule.getModuleIdentifier()
- << ": Missing summary for " << CalledFunctionName
- << ", error at import?\n");
- llvm_unreachable("Missing summary");
+ if (Threshold < CalleeSummary->instCount()) {
+ DEBUG(dbgs() << "ignored! Target instruction count ("
----------------
This should be an assert as selectCallee already checked this condition.
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:155
@@ +154,3 @@
+ // exported from outside as well.
+ for (auto &CalleeEdge : CalleeSummary->edges()) {
+ auto CalleeGUID = CalleeEdge.first;
----------------
This only adds the called functions, I think you need to do the same for the refs().
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:165
@@ +164,3 @@
+ auto *Summary = GlobInfo->summary();
+ if (!Summary)
+ return false;
----------------
Can this be an assert?
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:167
@@ +166,3 @@
+ return false;
+ ;
+ if (Summary->modulePath() == ExportModulePath) {
----------------
Extraneous ";"?
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:238
@@ +237,3 @@
+ auto *Summary = dyn_cast_or_null<FunctionSummary>(GlobInfo->summary());
+ if (!Summary)
+ continue;
----------------
Comment about ignoring global variables (which should be the only reason this is null?).
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:241
@@ +240,3 @@
+ DEBUG(dbgs() << "Adding definition: Module '" << Summary->modulePath()
+ << "' defines '" << GlobalList.first << "'\n");
+ Module2FunctionInfoMap[Summary->modulePath()][GUID] = Summary;
----------------
Use GUID instead of GlobalList.first for consistency/clarity.
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:255
@@ -294,1 +254,3 @@
+ << ImportsForModule.size() << " functions and exports "
+ << ExportLists.size() << "\n");
}
----------------
Isn't this the number of modules being exported from in total? I.e. the size of this will monotonically increase as we invoke ComputeImportForModule across all the modules. Message is confusing - this isn't the number of exports from this module.
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:291
@@ +290,3 @@
+ // Find the globals to import
+ DenseSet<const GlobalValue *> FunctionsToImport;
+ for (auto &GV : *SrcModule) {
----------------
GlobalsToImport here and in map name since it covers gvars now too.
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:302
@@ +301,3 @@
+ auto GUID = Function::getGUID(Function::getGlobalIdentifier(
+ GV.getName(), GV.getLinkage(), SrcModule->getModuleIdentifier()));
+ if (ImportGUIDs.count(GUID)) {
----------------
Use your new getGUID() here and in alias handling below too. Also, needs merge as the existing static getGUID() has moved to GlobalValue from Function.
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:307
@@ +306,3 @@
+ const GlobalObject *GO = GV.getBaseObject();
+ if (!GO->hasLinkOnceODRLinkage())
+ continue;
----------------
Add comment about why this is done?
================
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.
----------------
This patch seems to undo the renaming done in r263513.
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:403
@@ -429,4 +402,3 @@
std::string Error;
- IndexPtr =
- getModuleSummaryIndexForFile(SummaryFile, Error, diagnosticHandler);
+ IndexPtr = getFunctionIndexForFile(SummaryFile, Error, diagnosticHandler);
if (!IndexPtr) {
----------------
Another renaming that was undone by patch (bad merge?)
================
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()];
----------------
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.
================
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()
----------------
Why these changes of using .bc instead of .ll? Is it needed or at least independent and can be committed separately?
http://reviews.llvm.org/D18343
More information about the llvm-commits
mailing list