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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

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.

>> This seems pretty straightforward using `ConstantExpr::getWithOperands`. This also takes us one step closer to the canonical alias form we discussed last year: we would just need to apply the same canonicalization *before* writing the bitcode file.
> 
> Yes this alias canonicalization would also fix the issue, before the summary is generated. Unfortunately it's not something I have had time to work on since that first attempt.

Yes but note that I am not proposing that we canonicalize everywhere right now. We just need to canonicalize here: http://llvm-cs.pcc.me.uk/lib/LTO/LTOBackend.cpp#490 (before `dropDeadSymbols`).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54507