[PATCH] D18908: ThinLTO: Move the ODR resolution to be based purely on the summary.

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: lib/IR/ModuleSummaryIndex.cpp:81
@@ +80,3 @@
+      if (isa<GlobalVarSummary>(Summary))
+        /// Ignore global variable, focus on functions
+        continue;
----------------
tejohnson wrote:
> 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.


http://reviews.llvm.org/D18908