This is a requirement for the cache handling in D18494
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.
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. =)
(GUID -> Summary) ?
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.
Maybe name this map consistently throughout? How about ModuleToDefinedGVSummaries?
Why? This seems like a regression in functionality from current HEAD
Just a note for future cleanup - this file uses inconsistent function capitalization.
I changed it late and didn't clang-format it
This should be almost "code motion", please look at the changes in FunctionImport.cpp in this patch, the comment comes from there.
OK, will fix in a separate patch
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?
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.