jdoerfert accepted this revision.
jdoerfert added a comment.
LG from my side
================
Comment at: clang/test/Headers/Inputs/include/cstdlib:29
float fabs(float __x) { return __builtin_fabs(__x); }
+#endif
----------------
JonChesterfield wrote:
> 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.
> > 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.
@estewart08 Feel free to provide me with something that doesn't work even as
this goes in. It sounded you had some ideas and I'd like to look into that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104904/new/
https://reviews.llvm.org/D104904
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
