[PATCH] D60421: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols

Fri Apr 12 14:38:28 PDT 2019

steven_wu added a comment.

In D60421#1464989 <https://reviews.llvm.org/D60421#1464989>, @bd1976llvm wrote:

> Sorry for the delay in replying. I was trying to understand if the upgrade path was important. Comments inline:
>
> In D60421#1461442 <https://reviews.llvm.org/D60421#1461442>, @steven_wu wrote:
>
> > In D60421#1461407 <https://reviews.llvm.org/D60421#1461407>, @bd1976llvm wrote:
> >
> > > Hi Steven,
> > >
> > > I missed that this work was being done - sorry.
> > >
> > > Currently the legacy api does not require the IRSymtab to be written to the bitcode files so there is potentially a performance degradation due to this approach and the generated bitcode files will be larger, did you measure that?
> >
> >
> > The legacy API is not writing out IRSymtab but build that in memory during code generation. The only requirement change is that now data layout is required for thin LTO .
>
>
> My understanding is that the on disk bitcode contains a binary representation of the IRSymtab and this is read  into memory when loading the file (which  is why the upgrade path is more expensive than the no-upgrade path). I am saying that currently the legacy api does not require the IRSymtab to be written to disk.  Writing it may cost more time and make the input files bigger.

My patch didn't change that. IRSymtabs are built during thin-link time, hold in memory and it is not written out the disk. Any additional IO during thinLTO can potentially be a huge overhead so we are definitely introducing that. Am I misunderstanding your question?

>>> There is also a hidden performance cost that if the bitcode needs upgrading then the IRSymtab must be regenerated which can be expensive.
>> 
>> I did a simple performance test in https://reviews.llvm.org/D60226. Yes, the cost of hitting the upgrade path is quite significant (50% thin link time increase) but not measurable if no upgrade is hit. I don’t have big concerns over this. Let me know if you think it is harmful.
> 
> My concern is with patch releases. These might hit the upgrade path for thirdparty bitcode libraries. This might be inconvenient for developers to work around. I will try to get back to you on Monday after I have gathered more information on this.

Sure. @dexonsmith, do you have any concerns on this?

>> 
>> 
>>> An alternative approach is for the legacy api to get the list of used symbols from the bitcode and expose them via the lto_symbol_attributes; the system linker can then add "used" symbols to the preserve list directly. This seems more in-keeping with the original design IMO.
>> 
>> That was one of the proposals I had but some change needs to made to encode and convey this info. See previous review for more info.
> 
> I can have a go at implementing this to see what would be involved, if that would help?

Sure. I think my initial proposal was in the bug report: https://bugs.llvm.org/show_bug.cgi?id=41236
You can take a look and see if you agree with our discussion.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60421