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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

OpenMP side looks reasonable.



================
Comment at: clang/lib/Headers/__clang_hip_cmath.h:96
+__DEVICE__ __CONSTEXPR__ bool isnan(float __x) { return ::__isnanf(__x); }
+__DEVICE__ __CONSTEXPR__ bool isnan(double __x) { return ::__isnan(__x); }
 
----------------
^ This is how OpenMP resolves the overload issue wrt. different return types.


================
Comment at: clang/lib/Headers/__clang_hip_math.h:35
+#ifdef __OPENMP_AMDGCN__
+#define __RETURN_TYPE int
+#else
----------------
pdhaliwal wrote:
> jdoerfert wrote:
> > JonChesterfield wrote:
> > > I'd expect openmp to match the cplusplus/c distinction here, as openmp works on C source
> > ^ Agreed. Though, we use a different trick because it's unfortunately not as straight forward always and can be decided based on the C vs C++.
> This is somewhat tricky. Since declaration of `__finite/__isnan /__isinff` is with int return type in standard library (and the corresponding methods in C++ seems to be isfinite, isnan and isinf with bool return type), the compiler fails to resolve these functions when using bool. I don't know how HIP is working.
> 
> __RETURN_TYPE macro is only being used with the following methods:
> 1. __finite
> 2. __isnan
> 3. __isinf
> 4. __signbit
> 
> and with the corresponding float versions.
I marked the code above that actually overloads these functions in OpenMP (or better the versions w/o underscores) such that the system can have either version and it should work fine.


================
Comment at: clang/test/Headers/Inputs/include/cstdlib:29
 float fabs(float __x) { return __builtin_fabs(__x); }
+#endif
 
----------------
That seems to be fundamentally broken then, but let's see, maybe it will somehow work anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104904