[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.


================
Comment at: clang/test/Headers/Inputs/include/cstdlib:29
 float fabs(float __x) { return __builtin_fabs(__x); }
+#endif
 
----------------
jdoerfert wrote:
> estewart08 wrote:
> > JonChesterfield wrote:
> > > jdoerfert wrote:
> > > > That seems to be fundamentally broken then, but let's see, maybe it will somehow work anyway.
> > > I thought fabs was in math, not stdlib. Not sure what this file is doing but the functions above are inline and fabs isn't
> > I am afraid this is just a workaround to get openmp_device_math_isnan.cpp to pass for AMDGCN. This stems from not having an #ifndef OPENMP_AMDGCN in __clang_hip_cmath.h where 'using ::fabs' is present. Currently, OPENMP_AMDGCN uses all of the overloaded functions created by the HIP macros where NVPTX does not use the CUDA overloads. This may be a new topic of discussion.
> > 
> > https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/__clang_cuda_cmath.h#L191
> > 
> > By using this ifndef, it seems NVPTX looses quite a few overloaded functions. Are these meant to eventually be present in openmp_wrappers/cmath? Not sure what issues @jdoerfert ran into with D75788.
> > By using this ifndef, it seems NVPTX looses quite a few overloaded functions. Are these meant to eventually be present in openmp_wrappers/cmath? Not sure what issues @jdoerfert ran into with D75788.
> 
> Can you provide an example that shows how we "loose" something? So an input and command line that should work but doesn't, or that should be compiled to something else. That would help me a lot.
TLDR, I think nvptx works here, but it's hard to be certain. I've put a few minutes into looking for something that doesn't work, then much longer trying to trace where the various functions come from, and have concluded that the hip cmath header diverging from the cuda cmath header is the root cause.

The list of functions near the top of `__clang_cuda_cmath.h` is a subset of libm, e.g.
```
__DEVICE__ float acos(float __x) { return ::acosf(__x); }
but no acosh
```
Later on in the file are:
```
__CUDA_CLANG_FN_INTEGER_OVERLOAD_1(double, acosh)
```
but these are guarded by `#ifndef __OPENMP_NVPTX__`, which suggests they are not included when using the header from openmp.

However, openmp_wrappers/cmath does include `__DEVICE__ float acosh(float __x) { return ::acoshf(__x); }` under the comment
`// Overloads not provided by the CUDA wrappers by by the CUDA system headers`

Finally there are some functions that are not in either list, such as fma(float,float,float), but which are nevertheless resolved, at a guess in a glibc header.

My current theory is that nvptx gets the set of functions right through a combination of cuda headers, clang cuda headers, clang openmp headers, system headers. At least, the half dozen I've tried work, and iirc it passes the OvO suite which I believe calls all of them. Wimplicit-float-conversion complains about a few but that seems minor.

Further, I think hip does not get this right, because the hip cmath header has diverged from the cuda one, and the amdgpu openmp implementation that tries to use the hip headers does not pass the OvO suite without some hacks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104904