[PATCH] D40747: [ThinLTO] Enable importing of aliases as copy of aliasee

[Peter Collingbourne via Phabricator via llvm-commits]

pcc accepted this revision.
pcc added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:127
+static cl::opt<bool>
+    ImportAllIndex("import-all-index",
+                   cl::desc("Import all external functions in index."));
----------------
tejohnson wrote:
> pcc wrote:
> > I see why this is needed for now, but it seems like there is a possibility that we could make the distributed combined summary format simpler by not including a summary at all but just the names of the globals that need to be imported.
> I took a quick look and it appears that there is one place we use other info out of the summaries for imported values: setting DSOLocal in FunctionImportGlobalProcessing::processGlobalForThinLTO. It's possible that we may add other flags that we want to propagate in the summary to be set on imported values too, so I'm not very enthusiastic on removing those summaries altogether.
It's possible that we could split what is currently the GlobalValueSummary into a summary part and a "resolution" part (the resolutions would be stored in a separate index), and then just store the resolutions in what is currently the summary file. This could not only make the flow of information more understandable, but it would allow us to break the 1:1 correspondence between summaries and resolutions, which is problematic for DSO local right now because it can apply to declarations as well as definitions (so right now we can end up dropping the DSO local bit on declarations that don't have summaries).


https://reviews.llvm.org/D40747