[PATCH] D61765: [OpenMP][Clang][BugFix] Split declares and math functions inclusion.

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Last issue I have (in addition to the check @tra suggested) is the order in which we include math.h and cstdlib. can you flip it in one of the test cases?



================
Comment at: lib/Headers/__clang_cuda_math_forward_declares.h:30-38
+#ifndef _OPENMP
+__DEVICE__ long abs(long);
+__DEVICE__ long long abs(long long);
+#else
+#ifndef __cplusplus
 __DEVICE__ long abs(long);
 __DEVICE__ long long abs(long long);
----------------
gtbercea wrote:
> tra wrote:
> > I'm not quite sure what's the idea here. It may be worth adding a comment.
> > 
> > It could also be expressed somewhat simpler:
> > 
> > ```
> > #if !(defined(_OPENMP) && defined(__cplusplus))
> > ...
> > #endif
> > ```
> > 
> When these two functions definitions are here or in the __clang_cuda_cmath.h header then I get the following error (adapted for the __clang_cuda_cmath.h case):
> 
> 
> ```
> /usr/lib/gcc/ppc64le-redhat-linux/4.8.5/../../../../include/c++/4.8.5/cstdlib:166:3: error: declaration conflicts with target of using declaration already in scope
>   abs(long __i) { return __builtin_labs(__i); }
>   ^
> /autofs/home/gbercea/patch-compiler/obj-release/lib/clang/9.0.0/include/__clang_cuda_cmath.h:40:17: note: target of using declaration
> __DEVICE__ long abs(long __n) { return ::labs(__n); }
>                 ^
> /usr/lib/gcc/ppc64le-redhat-linux/4.8.5/../../../../include/c++/4.8.5/cstdlib:122:11: note: using declaration
>   using ::abs;
>           ^
> /usr/lib/gcc/ppc64le-redhat-linux/4.8.5/../../../../include/c++/4.8.5/cstdlib:174:3: error: declaration conflicts with target of using declaration already in scope
>   abs(long long __x) { return __builtin_llabs (__x); }
>   ^
> /autofs/home/gbercea/patch-compiler/obj-release/lib/clang/9.0.0/include/__clang_cuda_cmath.h:39:22: note: target of using declaration
> __DEVICE__ long long abs(long long __n) { return ::llabs(__n); }
>                      ^
> /usr/lib/gcc/ppc64le-redhat-linux/4.8.5/../../../../include/c++/4.8.5/cstdlib:122:11: note: using declaration
>   using ::abs;
> ```
> 
> 
Long story short, we currently cannot use the overload trick through `__device__` and therefore *replace* (not augment) host math headers with the cuda versions which unfortunately mix std math functions with other functions that we don't want/need.


================
Comment at: lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h:10
+
+#if defined(__NVPTX__) && defined(_OPENMP)
+
----------------
tra wrote:
> You may want to add include guards.
> 
> I'd also make inclusion of the file in non-openmp compilation an error, if it makes sense for OpenMP. It does for CUDA.
That is something we should be able to do, error out if _OPENMP is not defined I mean.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765