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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 2 inline comments as done.
tejohnson added a comment.

In https://reviews.llvm.org/D40747#956983, @pcc wrote:

> In https://reviews.llvm.org/D40747#953993, @tejohnson wrote:
>
> > In https://reviews.llvm.org/D40747#947144, @pcc wrote:
> >
> > > Now that we basically treat alias summaries in the same way as their aliasees, I wonder whether we can simplify things by removing AliasSummary and summarizing aliases by creating a FunctionSummary or GlobalVariableSummary from the aliasee instead.
> >
> >
> > I'm not sure we can as there are other requirements for knowing the alias relationship. For example, we can't convert a non-prevailing alias and aliasee to available_externally (see thinLTOResolveWeakForLinkerGUID) - that is in the original module and not related to importing.
>
>
> That could be resolved by adding a HasAlias flag to GlobalValueSummary, no? We would set it on aliases and anything that is aliased.


Possible yes.

> Sorry for the delay, I was working on prototyping my suggested change to make sure that it would work (and so far it looks like it would). This is probably fine for now though.

Ok thanks. Let's consider that for a follow on change?



================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:127
+static cl::opt<bool>
+    ImportAllIndex("import-all-index",
+                   cl::desc("Import all external functions in index."));
----------------
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.


https://reviews.llvm.org/D40747