Page MenuHomePhabricator

ThinLTO: Move the ODR resolution to be based purely on the summary.
ClosedPublic

Authored by mehdi_amini on Apr 8 2016, 1:16 PM.

Details

Summary

This is a requirement for the cache handling in D18494

Diff Detail

Event Timeline

mehdi_amini updated this revision to Diff 53075.Apr 8 2016, 1:16 PM
mehdi_amini retitled this revision from to ThinLTO: Move the ODR resolution to be based purely on the summary..
mehdi_amini updated this object.
mehdi_amini added a reviewer: tejohnson.
mehdi_amini added a subscriber: llvm-commits.
mehdi_amini updated this revision to Diff 53085.Apr 8 2016, 2:00 PM

Minor cleanup

tejohnson edited edge metadata.Apr 8 2016, 2:33 PM

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.

mehdi_amini updated this revision to Diff 53089.Apr 8 2016, 2:37 PM
mehdi_amini edited edge metadata.

s/ResolvedORD/ResolvedODR/

In D18908#395912, @joker.eph wrote:

(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
196

Just a note for future cleanup - this file uses inconsistent function capitalization.

mehdi_amini updated this revision to Diff 53156.Apr 9 2016, 1:46 PM
mehdi_amini marked 5 inline comments as done.

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
196

OK, will fix in a separate patch

tejohnson added inline comments.Apr 11 2016, 6:40 AM
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?

mehdi_amini added inline comments.Apr 11 2016, 3:46 PM
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.

mehdi_amini marked 3 inline comments as done.Apr 15 2016, 1:47 AM

Anything else here?

lib/IR/ModuleSummaryIndex.cpp
81

Switch to full list and not just functions.

mehdi_amini marked an inline comment as done.

rebase

tejohnson accepted this revision.Apr 15 2016, 6:39 AM
tejohnson edited edge metadata.

lgtm

This revision is now accepted and ready to land.Apr 15 2016, 6:39 AM
mehdi_amini closed this revision.Apr 16 2016, 12:08 AM