[Openmp-commits] [PATCH] D70131: [libomptarget] Move supporti.h to support.cu

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

Assuming this doesn't break the build as the old version did, it still looks good to me.

In D70131#1742646 <https://reviews.llvm.org/D70131#1742646>, @JonChesterfield wrote:

> In D70131#1742637 <https://reviews.llvm.org/D70131#1742637>, @ABataev wrote:
>
> > What about the performance with this version of the runtime?
>
>
> The nvcc build will be slower as it doesn't support LTO. Fixable by changing to a unity build in a subsequent patch.


That is fine. Cleaning up the code structure and preparing for bigger changes is way more important than small variations in our untracked and non-release version performance.

> The bitcode library itself will be slightly less optimised as we combine the bitcode with llvm-link but don't run any subsequent optimisation passes on it. After the bitcode library is combined with the application, we'll have the same code as before. I don't think losing the always_inline attribute (which was implied by forceinline) makes any difference here as the functions are small.

Arguably, we need to look into issues like "not inlined" in device compilation separately and more generically. Marking all runtime functions always_inline but not the user code is less helpful than adjusting the inline threshold based on the target.

> We don't have any bitcode library level optimisations at present. I'd like to internalise the non-api symbols and run opt across it during the build. That's essentially a build time optimisation for the clients of the library and orthogonal to this change.

I'm unsure what you mean in the first part. Internalizing symbols and aggressively removing unneeded part of the device runtime is for sure what we want to do, maybe even for api symbols.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70131