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

Mon Apr 15 14:02:01 PDT 2019

bd1976llvm added a comment.

LGTM! Thanks for waiting for me to confirm. Comments inline:

In D60421#1465029 <https://reviews.llvm.org/D60421#1465029>, @steven_wu wrote:

> 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?

Sorry for the confusion. I see that this patch hasn't changed the behavior. This is because the IRSymtab is always written to the bitcode files. I imagined that, downstream, if you are shipping a toolchain that only uses the legacy api you would have disabled the generation of the IRSymtab, as it is not required. That's why I thought that there would be an impact from requiring the IRSymtab.

> 
> 
>>>> 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?

As things are now, this patch would mean that customers who receive patch releases would hit the upgrade path. However, we think that by the time we ship a toolchain containing this patch, we should either be able to change our release processes so that this is not an issue (by guaranteeing that patch releases will not need to upgrade the IRSymtab), or we will commit to improving the performance of the upgrade path. Therefore, this is no longer a concern for me.

> 
> 
>>> 
>>> 
>>>> 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.

Thanks. Actually, after looking at this more, I want to accept this patch rather than any of the options that you list on the bug. Using the IRSymtab is a departure from the original design of the legacy api... but I now believe that this is a "good thing", as it means that the legacy and the resolution api's will use the same mechanism. I would like the legacy api and the resolution api to share as much code as possible, as this will improve the situation with maintaining and extending LTO. I am also interested in speeding up access to the "llvm.linker.options" metadata from the legacy api. In Sony we use llvm.linker.options for "dependent libraries" - #pragma comment(lib, ...). Currently, accessing this metadata is slow; but, if we use the IRSymtab access the dependent libraries, then we will be able to increase performance without having to make any changes to the bitcode loader.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60421