[Openmp-commits] [PATCH] D126701: [Libomptarget] Do not use retaining attributes for the static library

[Joseph Huber via Phabricator via Openmp-commits]

jhuber6 added a comment.

In D126701#3563330 <https://reviews.llvm.org/D126701#3563330>, @JonChesterfield wrote:

> Removing / reducing the keepalive hack seems like a good thing but I expect -DLIBOMPTARGET_BC_TARGET to come back and bite us. It means putting (slightly) different bitcode in a static library vs linking the bitcode directly and I expect that'll lead to bugs that repro on one config and not the other.
>
> Instead of that, let's go with whichever setup works best for LTO and accept a minor performance regression on the non-LTO case. That gets us identical bitcode on each path and a straightforward message for users about choosing between compile time cost and runtime performance.

I don't think that will work, necessary functions will be optimized out otherwise. The best solution is to just not build the bitcode version in the first place and only use the static library. But this will tank performance pretty heavily on non-LTO builds since we can't inline runtime functions or optimize them together anymore. In the future we can make LTO the default since it improves performance in almost every case, but I don't want to make that switch right now. This is kind of the short workaround so we can get better IR with LTO without completely deleting the bitcode library. I think it'll be fine to have a slightly different version for the two methods. If one fails we can always tell them to use LTO until that's the default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126701