[PATCH] D54507: [ThinLTO] Handle chains of aliases

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In D54507#1328447 <https://reviews.llvm.org/D54507#1328447>, @tejohnson wrote:

> In D54507#1327677 <https://reviews.llvm.org/D54507#1327677>, @pcc wrote:
>
> > In D54507#1327609 <https://reviews.llvm.org/D54507#1327609>, @tejohnson wrote:
> >
> > > In D54507#1327573 <https://reviews.llvm.org/D54507#1327573>, @pcc wrote:
> > >
> > > > In D54507#1327487 <https://reviews.llvm.org/D54507#1327487>, @tejohnson wrote:
> > > >
> > > > > In D54507#1327394 <https://reviews.llvm.org/D54507#1327394>, @pcc wrote:
> > > > >
> > > > > > Isn't this just a bug in promotion? It should probably internalize any non-prevailing aliases like lib/Linker does.
> > > > >
> > > > >
> > > > > Can you clarify? The summary doesn't contain any references to the intermediate aliasee so it looks completely unreferenced.
> > > >
> > > >
> > > > And indeed it isn't. The important thing to keep alive is the base object, which is what the code already does.
> > > >
> > > > My first idea was that we would change the code in `convertToDeclaration` here:
> > > >  http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionImport.cpp#923
> > > >  so that we would RAUW all alias references to point to the external and all non-alias references to point to the aliasee. But unfortunately it doesn't seem easy to implement "replace some uses with" on constants.
> > > >
> > > > Probably the simplest fix is to make sure that each aliasee is canonicalized to point to the base object before calling `convertToDeclaration`.
> > >
> > >
> > > Do you mean make sure that each "alias" is canonicalized to point to the base object? I.e. if we have an alias relationship "A aliases B aliases C", then convert it to "A aliases C" and "B aliases C"?
> >
> >
> > Yes, exactly.
> >
> > > Presumably that would fix the issue, but the summary is still not accurately reflecting the IR (although not sure where else it would be problematic).
> >
> > I don't think it would be problematic. In this case the summary contains the more accurate representation of reality because references to globals in alias definitions, unlike all other references, are not subject to linker preemption, and we shouldn't allow the less-accurate representation in the IR to leak anywhere else.
>
>
> Philosophically, I think the summary should summarize the IR that we have (in the bitcode file), not the IR that we want. So I'm not completely comfortable with this approach - I'm actually thinking it may be better to add a skeleton alias canonicalization pass (taken from that draft alias canonicalization patch D29781 <https://reviews.llvm.org/D29781>) that is invoked during thinlto preparation. For now, it can just do this flatting of alias chains, and eventually expanded into the full canonicalization (and invoked in all paths as described in that patch description). I don't want to sign up to do the whole hog canonicalization currently since this bug only shows up at -O0 and I don't have the bandwidth to focus on it ATM. Does that seem like a reasonable interim approach?


Sounds reasonable enough. It seems a little better to do this during LTO because it would allow us to upgrade old bitcode but I'm happy for it to be a pass as well. I left some comments on D29781 <https://reviews.llvm.org/D29781> about the overall approach there.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54507/new/

https://reviews.llvm.org/D54507