[PATCH] D60907: [OpenMP] Add math functions support in OpenMP offloading

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D60907#1483615 <https://reviews.llvm.org/D60907#1483615>, @hfinkel wrote:

> In D60907#1479370 <https://reviews.llvm.org/D60907#1479370>, @gtbercea wrote:
>
> > In D60907#1479142 <https://reviews.llvm.org/D60907#1479142>, @hfinkel wrote:
> >
> > > In D60907#1479118 <https://reviews.llvm.org/D60907#1479118>, @gtbercea wrote:
> > >
> > > > Ping @hfinkel @tra
> > >
> > >
> > > The last two comments in D47849 <https://reviews.llvm.org/D47849> indicated exploration of a different approach, and one which still seems superior to this one. Can you please comment on why you're now pursuing this approach instead?
> >
> >
> > ...
> >
> > Hal, as far as I can tell, this solution is similar to yours but with a slightly different implementation. If there are particular aspects about this patch you would like to discuss/give feedback on please let me know.
>
>
> The solution I suggested had the advantages of:
>
> 1. Being able to directly reuse the code in `__clang_cuda_device_functions.h`. On the other hand, using this solution we need to implement a wrapper function for every math function. When `__clang_cuda_device_functions.h` is updated, we need to update the OpenMP wrapper as well.


I'd even go as far as to argue that `__clang_cuda_device_functions.h` should include the internal math.h wrapper to get all math functions. See also the next comment.

> 2. Providing access to wrappers for other CUDA intrinsics in a natural way (e.g., rnorm3d) [it looks a bit nicer to provide a host version of rnorm3d than __nv_rnorm3d in user code].

@hfinkel 
I don't see why you want to mix CUDA intrinsics with math.h overloads. I added a rough outline of how I imagined the internal math.h header to look like as a comment in D47849 <https://reviews.llvm.org/D47849>. Could you elaborate how that differs from what you imagine and how the other intrinsics come in?

> 3. Being similar to the "declare variant" functionality from OpenMP 5, and thus, I suspect, closer to the solution we'll eventually be able to apply in a standard way to all targets.

I can see this.

>> This solution is following Alexey's suggestions. This solution allows the optimization of math calls if they apply (example: pow(x,2) => x*x ) which was one of the issues in the previous solution I implemented.
> 
> So we're also missing that optimization for CUDA code when compiling with Clang? Isn't this also something that, regardless, should be fixed?

Maybe through a general built-in recognition and lowering into target specific implementations/intrinsics late again?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60907