This is a requirement for the cache handling in D18494
Details
Diff Detail
Event Timeline
Just took a quick initial scan, one typo issue throughout: s/ResolvedORD/ResolvedODR/.
Also, what is the main gist of this change? The old code is refactored but it isn't immediately obvious to me what is different about the end state.
lib/LTO/ThinLTOCodeGenerator.cpp | ||
---|---|---|
171 | typo: ResolvedORD ->ResolvedODR |
(I'm not sure what you're asking about let me know if this does not answer)
Previously we loaded the IR in memory and iterated through the Module:
- looking at linkage on the GlovalValue, deciding if we need to change it.
- changing it immediately
Now the first phase is performed purely on the summary, without loading the IR.
Got it. I looked at the summary but somehow not the title, was in a hurry. =)
include/llvm/Transforms/IPO/FunctionImport.h | ||
---|---|---|
63 | (GUID -> Summary) ? | |
73 | Looks like this new line is unnecessary (the next line's parameter appears to start at the column after "(", but maybe my eyes are deceiving me. | |
76 | Maybe name this map consistently throughout? How about ModuleToDefinedGVSummaries? | |
lib/IR/ModuleSummaryIndex.cpp | ||
81 | Why? This seems like a regression in functionality from current HEAD | |
lib/LTO/ThinLTOCodeGenerator.cpp | ||
192 | Just a note for future cleanup - this file uses inconsistent function capitalization. |
Address comments.
include/llvm/Transforms/IPO/FunctionImport.h | ||
---|---|---|
73 | I changed it late and didn't clang-format it | |
lib/IR/ModuleSummaryIndex.cpp | ||
81 | This should be almost "code motion", please look at the changes in FunctionImport.cpp in this patch, the comment comes from there. | |
lib/LTO/ThinLTOCodeGenerator.cpp | ||
192 | OK, will fix in a separate patch |
lib/IR/ModuleSummaryIndex.cpp | ||
---|---|---|
81 | I know it came from there, so from the standpoint of ComputeCrossModuleImport this is not a change or regression in functionality. However, for the ResolveODR code it is. Previously, ResolveODR on the module would walk the globals() and invoke the per-symbol ResolveODR on each. Now, because no global variables are put into the map, ResolveODR will never encounter a global variable and therefore no global variables will end up in the ResolvedODR map. So AFAICT the globals() handling in fixupODR will never actually encounter a global variable that is in this set, and no global variables will ever be fixed up. Looks like test/ThinLTO/X86/odr_resolution.ll does not contain any global variables, so that's why the change in functionality wasn't detected - can you add one there? |
lib/IR/ModuleSummaryIndex.cpp | ||
---|---|---|
81 | Oh yes, from the point of view of the ODR it is a change. However I intended it since this is a "compile time optimization" and emitting a global is not expensive. On the other hand reducing the "diff" between two links increases the likeliness to have a cache hit. |
Collect all the symbols defined in the module and not only the functions. This would cause trouble later for caching+internalization otherwise.
Anything else here?
lib/IR/ModuleSummaryIndex.cpp | ||
---|---|---|
81 | Switch to full list and not just functions. |
(GUID -> Summary) ?