Now that the summary contains the full reference/call graph, we can
replace the existing function importer that loads and inspect the IR
to iteratively walk the call graph by a traversal based purely on the
summary information. Decouple the actual importing decision from any
IR manipulation.
Details
Diff Detail
Event Timeline
Thanks! A few comments/questions below.
include/llvm/Transforms/IPO/FunctionImport.h | ||
---|---|---|
50 | 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. | |
60 | s/enty/entry/ | |
61 | 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. | |
lib/LTO/ThinLTOCodeGenerator.cpp | ||
374 | Initialize these two maps with the module count as done in ThinLTOCodeGenerator::crossModuleImport | |
lib/Transforms/IPO/FunctionImport.cpp | ||
69 |
| |
74 | 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. | |
111 | Give more meaningful message here? Maybe just as simple as "ignored! No qualifying callee with summary found." | |
127–128 | This should be an assert as selectCallee already checked this condition. | |
156 | This only adds the called functions, I think you need to do the same for the refs(). | |
166 | Can this be an assert? | |
168 | Extraneous ";"? | |
260 | Comment about ignoring global variables (which should be the only reason this is null?). | |
262 | 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. | |
263 | Use GUID instead of GlobalList.first for consistency/clarity. | |
302 | GlobalsToImport here and in map name since it covers gvars now too. | |
313 | 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. | |
318 | Add comment about why this is done? | |
364 | This patch seems to undo the renaming done in r263513. | |
416 | Another renaming that was undone by patch (bad merge?) | |
443 | 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. | |
test/Transforms/FunctionImport/adjustable_threshold.ll | ||
7 | Why these changes of using .bc instead of .ll? Is it needed or at least independent and can be committed separately? |
Thanks, I'll fix all of these comments, a few answers in the meantime.
lib/LTO/ThinLTOCodeGenerator.cpp | ||
---|---|---|
374 | Yes! I was waiting for my changes on the StringMap to be checked in (committed yesterday). | |
lib/Transforms/IPO/FunctionImport.cpp | ||
364 | Probably an issue when rebasing on top of your changes. | |
443 | You can call ComputeCrossModuleImport and get the same answer in every backend jobs. | |
test/Transforms/FunctionImport/adjustable_threshold.ll | ||
7 | It is needed: the summary will contains the module path as a string pointing to the .bc. |
lib/Transforms/IPO/FunctionImport.cpp | ||
---|---|---|
67 | Why not passing a reference of the Edge to 'selectCallee'. We will need to look at profile data in the future. | |
69 | In LLVM, comdat function's profile counters will also be put into comdat, so there should be no need to differentiate based on profile availability. |
lib/Transforms/IPO/FunctionImport.cpp | ||
---|---|---|
67 | I think the profile info will not impact *which* callee you select right? What do you expect to do with this information at this level? |
lib/Transforms/IPO/FunctionImport.cpp | ||
---|---|---|
67 | It will impact importing strategy. For instance increased threshold for hot callsites and reduced threshold for extreme cold ones. The summary Edge can also contain other nice bits (such as has constant argument etc) which will help importing more precisely. |
lib/Transforms/IPO/FunctionImport.cpp | ||
---|---|---|
67 | I agree, it is not clear to me that exposing the "Edge" here is the right abstraction (one could modifying the Threshold at call site to include a PGO bias for instance, even if it is probably not the right thing either). I think that I can safely leave this for the larger refactoring work that will follow when we will rework the cost model for importing. |
lib/Transforms/IPO/FunctionImport.cpp | ||
---|---|---|
67 | Sounds fine to me. |
include/llvm/Transforms/IPO/FunctionImport.h | ||
---|---|---|
61 | Typedef solve it all :) I'll probably commit a subsequent cleanup to make GUID a typedef as well. | |
lib/Transforms/IPO/FunctionImport.cpp | ||
67 | Great, thanks David! | |
69 | Updated the comment. | |
74 | I think this was originally an assert and I figured later it was a legit case. Can't remember, will turn back into an assert for now. | |
156 | This was intentional, comment added. | |
318 | I have no idea, I remember copying it from what I was tracking in the ModuleLinker/IRMover. But can't retrieve it now, we want to import an alias to a global variable, should we import the global as well if it is link once? |
include/llvm/Transforms/IPO/FunctionImport.h | ||
---|---|---|
28 | s/function/functions | |
35 | Incomplete sentence "The value for" | |
38–39 | s/functions/function/ Also, not a map. So maybe ExportSetTy in addition to comment fix? | |
lib/Transforms/IPO/FunctionImport.cpp | ||
156 | 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. | |
319 | 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. |
Thanks, good catch here. Updated!
lib/Transforms/IPO/FunctionImport.cpp | ||
---|---|---|
156 | Oh, good point, my previous answer was totally wrong, I thought we were looking at the "import" list here. When I wrote the original code I intended Summary->edge() to iterate over both calls() and refs(). Fixed! | |
319 | Updated comment |
lib/Transforms/IPO/FunctionImport.cpp | ||
---|---|---|
119 | This can be removed now. |
s/function/functions
s/. each/. Each/