[Openmp-commits] [PATCH] D121467: [Libomptarget] Create device math wrappers

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

In D121467#3375505 <https://reviews.llvm.org/D121467#3375505>, @JonChesterfield wrote:

> OK, I think I follow.
>
> What we have is:
> 1/ Long list of libm symbols as standardised with some ad hoc extra ones
> 2/ Header file mapping libm symbols onto cuda functions and intrinsics
> 3/ Header file mapping libm symbols onto hip (well, ocml) functions and intrinsics
>
> What we want is:
> Applications #include math.h and stuff works. Maybe an extra #include math_gpu.h containing the ad hoc extra ones.
> Optimisations in LLVM that are tied to the C symbol name work
>
> Thus, we instantiate a bitcode library that maps libm + some extra symbols onto the cuda/ocml library by reusing the 'header' file with some macro hackery to make it do the right thing across libraries.
>
> We might want to split OpenMPMath.h into standard and extensions, because cuda put the extensions in the global namespace and some applications written in openmp are going to define the same symbols.

Agreed. We put that on the TODO list.

> Strategy looks sound to me. It's unfortunate that the symbol remap tables are written as C++ header files but at least they already exist. Copy&pasting them into openmp is going to be a maintenance disaster almost immediately, I think we need to add some macros to the headers in clang and include them via CMakeLists setting the include path appropriately.

We want a single version of these. All but sin -> {nv,ocml,...}_sin should be unique. That is not the case right now. This stuff is part of the unique set of files and should not live in openmp/... but rather clang/deviceLibs or similar.
For now, assume this review is just so we have a place to put the things. We are still making things work and once we have we can move stuff to the proper place.

> The review of adding those macros can include a link to this diff to show that said macros are the lesser of two evils.

Changing to macros is something we can do and might even allow us to cut down the files by one.
With something like MAP(from, to, return_type, arg_types) we might be able to create the wrappers from sin -> __llvm_gpu_sin, and from __llvm_gpu_sin -> sin, and from sin -> __{nv,ocml}_sin.

All that said, the macros could have been done before, and can be done after this rewrite.

> AMD have already done something rather like this for Fortran (which cannot use the C++ headers), see https://github.com/RadeonOpenCompute/llvm-project/blob/amd-stg-open/openmp/libomptarget/libm/src/libm.c with corresponding macros added to the clang headers.
>
> I'm therefore marking this 'requested changes', with the proviso that the change I'm requesting is that we add more ugliness to the clang headers to make this work out OK.
>
> We might want to put a file called math.h in the override directory and not #include_next the system math.h on the gpu.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121467