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

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC360626: [OpenMP][Clang][BugFix] Split declares and math 
functions inclusion. (authored by gbercea, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D61765?vs=199304=199340#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D61765

Files:
  lib/Driver/ToolChains/Clang.cpp
  lib/Headers/CMakeLists.txt
  lib/Headers/__clang_cuda_cmath.h
  lib/Headers/__clang_cuda_device_functions.h
  lib/Headers/__clang_cuda_math_forward_declares.h
  lib/Headers/openmp_wrappers/__clang_openmp_math.h
  lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  lib/Headers/openmp_wrappers/cmath
  lib/Headers/openmp_wrappers/math.h
  test/Headers/Inputs/include/cstdlib
  test/Headers/nvptx_device_cmath_functions.c
  test/Headers/nvptx_device_cmath_functions.cpp
  test/Headers/nvptx_device_math_functions.c
  test/Headers/nvptx_device_math_functions.cpp

Index: test/Headers/nvptx_device_cmath_functions.cpp
===
--- test/Headers/nvptx_device_cmath_functions.cpp
+++ test/Headers/nvptx_device_cmath_functions.cpp
@@ -4,9 +4,10 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include cmath -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
+#include 
 
 void test_sqrt(double a1) {
   #pragma omp target
Index: test/Headers/nvptx_device_math_functions.c
===
--- test/Headers/nvptx_device_math_functions.c
+++ test/Headers/nvptx_device_math_functions.c
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: test/Headers/nvptx_device_cmath_functions.c
===
--- test/Headers/nvptx_device_cmath_functions.c
+++ test/Headers/nvptx_device_cmath_functions.c
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include cmath -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda 

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

[Artem Belevich via Phabricator via cfe-commits]

tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.

In D61765#1500457 , @gtbercea wrote:

> This won't affect CUDA in any way, all we have added is OpenMP specific.


LGTM for CUDA. I'll leave the question of testing with libc++ to someone more 
familiar with OpenMP.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D61765#1500351 , @gtbercea wrote:

> - Exclude clock functions. Reverse inclusion order.


LGTM from my side. I don't have strong feelings about testing libc++ now, 
though it is probably a good idea to have such a testbed.
I agree this should not infer with CUDA (at least that is our intention).

P.S.
There will be a ticket to add the OpenMP variant support we need (similar to 
the `__device__` in CUDA) will be written by Tom Scogland and me for a first 
vote already in this meeting.
That is what we will then need to implement.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea added a comment.

In D61765#1500446 , @tra wrote:

> In D61765#1500309 , @gtbercea wrote:
>
> > As soon as libc++ the limits header included in
> >
> >   __clang_cuda_cmath.h:15
> >   ``` is not found:
> >  
> >  
> >
> >
> > __clang_cuda_cmath.h:15:10: fatal error: 'limits' file not found
> >  #include 
> >
> >   Not even CUDA works actually so I'm not sure what the best answer to this 
> > problem is.
>
>
> Could you give me more details on how you've got this error?
>
> If this change breaks CUDA compilation with libc++, that's going to be a 
> problem. Currently CUDA and clang's headers we ship do work with both libc++ 
> and few versions of libstdc++:
>  E.g: 
> http://lab.llvm.org:8011/builders/clang-cuda-build/builds/33364/steps/ninja%20build%20simple%20CUDA%20tests/logs/stdio


This won't affect CUDA in any way, all we have added is OpenMP specific.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea added a comment.

In D61765#1500446 , @tra wrote:

> In D61765#1500309 , @gtbercea wrote:
>
> > As soon as libc++ the limits header included in
> >
> >   __clang_cuda_cmath.h:15
> >   ``` is not found:
> >  
> >  
> >
> >
> > __clang_cuda_cmath.h:15:10: fatal error: 'limits' file not found
> >  #include 
> >
> >   Not even CUDA works actually so I'm not sure what the best answer to this 
> > problem is.
>
>
> Could you give me more details on how you've got this error?
>
> If this change breaks CUDA compilation with libc++, that's going to be a 
> problem. Currently CUDA and clang's headers we ship do work with both libc++ 
> and few versions of libstdc++:
>  E.g: 
> http://lab.llvm.org:8011/builders/clang-cuda-build/builds/33364/steps/ninja%20build%20simple%20CUDA%20tests/logs/stdio


It's an error on my side, I don't have libc++ installed so trying to use it 
will come up with header not found errors.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D61765#1500309 , @gtbercea wrote:

> As soon as libc++ the limits header included in
>
>   __clang_cuda_cmath.h:15
>   ``` is not found:
>  
>  
>
>
> __clang_cuda_cmath.h:15:10: fatal error: 'limits' file not found
>  #include 
>
>   Not even CUDA works actually so I'm not sure what the best answer to this 
> problem is.


Could you give me more details on how you've got this error?

If this change breaks CUDA compilation with libc++, that's going to be a 
problem. Currently CUDA and clang's headers we ship do work with both libc++ 
and few versions of libstdc++:
E.g: 
http://lab.llvm.org:8011/builders/clang-cuda-build/builds/33364/steps/ninja%20build%20simple%20CUDA%20tests/logs/stdio


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea updated this revision to Diff 199304.
gtbercea added a comment.

- Exclude clock functions. Reverse inclusion order.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765

Files:
  lib/Driver/ToolChains/Clang.cpp
  lib/Headers/CMakeLists.txt
  lib/Headers/__clang_cuda_cmath.h
  lib/Headers/__clang_cuda_device_functions.h
  lib/Headers/__clang_cuda_math_forward_declares.h
  lib/Headers/openmp_wrappers/__clang_openmp_math.h
  lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  lib/Headers/openmp_wrappers/cmath
  lib/Headers/openmp_wrappers/math.h
  test/Headers/Inputs/include/cstdlib
  test/Headers/nvptx_device_cmath_functions.c
  test/Headers/nvptx_device_cmath_functions.cpp
  test/Headers/nvptx_device_math_functions.c
  test/Headers/nvptx_device_math_functions.cpp

Index: test/Headers/nvptx_device_math_functions.cpp
===
--- test/Headers/nvptx_device_math_functions.cpp
+++ test/Headers/nvptx_device_math_functions.cpp
@@ -4,8 +4,9 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -include cstdlib -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
+#include 
 #include 
 
 void test_sqrt(double a1) {
Index: test/Headers/nvptx_device_math_functions.c
===
--- test/Headers/nvptx_device_math_functions.c
+++ test/Headers/nvptx_device_math_functions.c
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: test/Headers/nvptx_device_cmath_functions.cpp
===
--- test/Headers/nvptx_device_cmath_functions.cpp
+++ test/Headers/nvptx_device_cmath_functions.cpp
@@ -4,9 +4,10 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include cmath -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda 

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea added a comment.

In D61765#1500233 , @tra wrote:

> In D61765#1499957 , @jdoerfert wrote:
>
> > Two small changes and then it is fine with me. @tra ?
>
>
> LGTM in general. I would still like to confirm that the changes work with 
> libc++.


As soon as libc++ the limits header included in

  __clang_cuda_cmath.h:15
  ``` is not found:

__clang_cuda_cmath.h:15:10: fatal error: 'limits' file not found
#include 

  Not even CUDA works actually so I'm not sure what the best answer to this 
problem is.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D61765#1499957 , @jdoerfert wrote:

> Two small changes and then it is fine with me. @tra ?


LGTM in general. I would still like to confirm that the changes work with 
libc++.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Two small changes and then it is fine with me. @tra ?

1. we need to use ifdef to not define clock
2. Can you switch the include order in 
`test/Headers/nvptx_device_math_functions.cpp`?

P.S. I'm currently at the OpenMP standard meeting to get the OpenMP variants 
fixed.
Once done, we should prioritize the implementation.
Excluding non-math functions in the cuda headers is not perfect...


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



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:
> > tra wrote:
> > > gtbercea wrote:
> > > > jdoerfert wrote:
> > > > > 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.
> > > > This doesn't seem to be happening in the CUDA case. My suspicion is 
> > > > it's because of the __device__ attribute.
> > > It looks like until OpenMP supports some sort of target-based overloading 
> > > this will not play nicely with libstdc++. 
> > > Did you, by any chance, check if the header works with libc++ ? I wonder 
> > > if we may encounter more conflicts like these.
> > Correct. `__device__` functions overload whatever `(implicitly)__host__` 
> > functions declared by the standard library, so they coexist w/o problems. 
> > Usually. host/device implementation nuances are still observable.
> > 
> Just did a few quick tests with libstdc++ and it was all good. 
How about `libc++`? The idea is to make sure the change works with both 
libraries.




Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea marked an inline comment as done.
gtbercea added inline comments.



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);

tra wrote:
> tra wrote:
> > gtbercea wrote:
> > > jdoerfert wrote:
> > > > 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.
> > > This doesn't seem to be happening in the CUDA case. My suspicion is it's 
> > > because of the __device__ attribute.
> > It looks like until OpenMP supports some sort of target-based overloading 
> > this will not play nicely with libstdc++. 
> > Did you, by any chance, check if the header works with libc++ ? I wonder if 
> > we may encounter more conflicts like these.
> Correct. `__device__` functions overload whatever `(implicitly)__host__` 
> functions declared by the standard library, so they coexist w/o problems. 
> Usually. host/device implementation nuances are still observable.
> 
Just did a few quick tests with libstdc++ and it was all good. 


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



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:
> jdoerfert wrote:
> > 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.
> This doesn't seem to be happening in the CUDA case. My suspicion is it's 
> because of the __device__ attribute.
It looks like until OpenMP supports some sort of target-based overloading this 
will not play nicely with libstdc++. 
Did you, by any chance, check if the header works with libc++ ? I wonder if we 
may encounter more conflicts like these.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea updated this revision to Diff 199080.
gtbercea added a comment.

- Error if not in OpenMP.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765

Files:
  lib/Driver/ToolChains/Clang.cpp
  lib/Headers/CMakeLists.txt
  lib/Headers/__clang_cuda_cmath.h
  lib/Headers/__clang_cuda_math_forward_declares.h
  lib/Headers/openmp_wrappers/__clang_openmp_math.h
  lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  lib/Headers/openmp_wrappers/cmath
  lib/Headers/openmp_wrappers/math.h
  test/Headers/Inputs/include/cstdlib
  test/Headers/nvptx_device_cmath_functions.c
  test/Headers/nvptx_device_cmath_functions.cpp
  test/Headers/nvptx_device_math_functions.c
  test/Headers/nvptx_device_math_functions.cpp

Index: test/Headers/nvptx_device_math_functions.cpp
===
--- test/Headers/nvptx_device_math_functions.cpp
+++ test/Headers/nvptx_device_math_functions.cpp
@@ -4,9 +4,11 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -include cstdlib -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
+//#include 
 #include 
+#include 
 
 void test_sqrt(double a1) {
   #pragma omp target
Index: test/Headers/nvptx_device_math_functions.c
===
--- test/Headers/nvptx_device_math_functions.c
+++ test/Headers/nvptx_device_math_functions.c
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: test/Headers/nvptx_device_cmath_functions.cpp
===
--- test/Headers/nvptx_device_cmath_functions.cpp
+++ test/Headers/nvptx_device_cmath_functions.cpp
@@ -4,9 +4,10 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include cmath -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device 

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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



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);

tra wrote:
> gtbercea wrote:
> > jdoerfert wrote:
> > > 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.
> > This doesn't seem to be happening in the CUDA case. My suspicion is it's 
> > because of the __device__ attribute.
> It looks like until OpenMP supports some sort of target-based overloading 
> this will not play nicely with libstdc++. 
> Did you, by any chance, check if the header works with libc++ ? I wonder if 
> we may encounter more conflicts like these.
Correct. `__device__` functions overload whatever `(implicitly)__host__` 
functions declared by the standard library, so they coexist w/o problems. 
Usually. host/device implementation nuances are still observable.



Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea marked an inline comment as done.
gtbercea added inline comments.



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);

jdoerfert wrote:
> 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.
This doesn't seem to be happening in the CUDA case. My suspicion is it's 
because of the __device__ attribute.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea marked an inline comment as done.
gtbercea added inline comments.



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);

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;
```




Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea updated this revision to Diff 199075.
gtbercea added a comment.

- Simplify conditions. Add guards.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765

Files:
  lib/Driver/ToolChains/Clang.cpp
  lib/Headers/CMakeLists.txt
  lib/Headers/__clang_cuda_cmath.h
  lib/Headers/__clang_cuda_math_forward_declares.h
  lib/Headers/openmp_wrappers/__clang_openmp_math.h
  lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  lib/Headers/openmp_wrappers/cmath
  lib/Headers/openmp_wrappers/math.h
  test/Headers/Inputs/include/cstdlib
  test/Headers/nvptx_device_cmath_functions.c
  test/Headers/nvptx_device_cmath_functions.cpp
  test/Headers/nvptx_device_math_functions.c
  test/Headers/nvptx_device_math_functions.cpp

Index: test/Headers/nvptx_device_math_functions.cpp
===
--- test/Headers/nvptx_device_math_functions.cpp
+++ test/Headers/nvptx_device_math_functions.cpp
@@ -4,9 +4,11 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -include cstdlib -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
+//#include 
 #include 
+#include 
 
 void test_sqrt(double a1) {
   #pragma omp target
Index: test/Headers/nvptx_device_math_functions.c
===
--- test/Headers/nvptx_device_math_functions.c
+++ test/Headers/nvptx_device_math_functions.c
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: test/Headers/nvptx_device_cmath_functions.cpp
===
--- test/Headers/nvptx_device_cmath_functions.cpp
+++ test/Headers/nvptx_device_cmath_functions.cpp
@@ -4,9 +4,10 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include cmath -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device 

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea updated this revision to Diff 199073.
gtbercea added a comment.

- Add tests. Exclude additional abs definition.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765

Files:
  lib/Driver/ToolChains/Clang.cpp
  lib/Headers/CMakeLists.txt
  lib/Headers/__clang_cuda_cmath.h
  lib/Headers/__clang_cuda_math_forward_declares.h
  lib/Headers/openmp_wrappers/__clang_openmp_math.h
  lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  lib/Headers/openmp_wrappers/cmath
  lib/Headers/openmp_wrappers/math.h
  test/Headers/Inputs/include/cstdlib
  test/Headers/nvptx_device_cmath_functions.c
  test/Headers/nvptx_device_cmath_functions.cpp
  test/Headers/nvptx_device_math_functions.c
  test/Headers/nvptx_device_math_functions.cpp

Index: test/Headers/nvptx_device_math_functions.cpp
===
--- test/Headers/nvptx_device_math_functions.cpp
+++ test/Headers/nvptx_device_math_functions.cpp
@@ -4,9 +4,11 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -include cstdlib -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
+//#include 
 #include 
+#include 
 
 void test_sqrt(double a1) {
   #pragma omp target
Index: test/Headers/nvptx_device_math_functions.c
===
--- test/Headers/nvptx_device_math_functions.c
+++ test/Headers/nvptx_device_math_functions.c
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: test/Headers/nvptx_device_cmath_functions.cpp
===
--- test/Headers/nvptx_device_cmath_functions.cpp
+++ test/Headers/nvptx_device_cmath_functions.cpp
@@ -4,9 +4,10 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include cmath -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s 

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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



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);

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
```




Comment at: lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h:10
+
+#if defined(__NVPTX__) && defined(_OPENMP)
+

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h:17
+  #include 
+  #include 
+#endif

gtbercea wrote:
> jdoerfert wrote:
> > Why do we need the stdlib includes again?
> They are both prone to abs inclusion.
> 
> We need them here to control the order in which they are included relative to 
> the forward_declares header.
I thought the "not defining abs" in __clang_cuda_math_forward_declares.h was 
the solution?


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea updated this revision to Diff 199062.
gtbercea added a comment.

- Clean test header.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765

Files:
  lib/Driver/ToolChains/Clang.cpp
  lib/Headers/CMakeLists.txt
  lib/Headers/__clang_cuda_math_forward_declares.h
  lib/Headers/openmp_wrappers/__clang_openmp_math.h
  lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  lib/Headers/openmp_wrappers/cmath
  lib/Headers/openmp_wrappers/math.h
  test/Headers/Inputs/include/cstdlib
  test/Headers/nvptx_device_cmath_functions.c
  test/Headers/nvptx_device_cmath_functions.cpp
  test/Headers/nvptx_device_math_functions.c
  test/Headers/nvptx_device_math_functions.cpp

Index: test/Headers/nvptx_device_math_functions.cpp
===
--- test/Headers/nvptx_device_math_functions.cpp
+++ test/Headers/nvptx_device_math_functions.cpp
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -include cstdlib -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: test/Headers/nvptx_device_math_functions.c
===
--- test/Headers/nvptx_device_math_functions.c
+++ test/Headers/nvptx_device_math_functions.c
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: test/Headers/nvptx_device_cmath_functions.cpp
===
--- test/Headers/nvptx_device_cmath_functions.cpp
+++ test/Headers/nvptx_device_cmath_functions.cpp
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include cmath -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: 

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea marked 2 inline comments as done.
gtbercea added inline comments.



Comment at: lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h:17
+  #include 
+  #include 
+#endif

jdoerfert wrote:
> Why do we need the stdlib includes again?
They are both prone to abs inclusion.

We need them here to control the order in which they are included relative to 
the forward_declares header.



Comment at: test/Headers/Inputs/include/cstdlib:2
+#pragma once
+typedef __SIZE_TYPE__ size_t;

jdoerfert wrote:
> Where is this used? Are there tests missing?
I'll remove it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

What about `abs` tests?




Comment at: lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h:17
+  #include 
+  #include 
+#endif

Why do we need the stdlib includes again?



Comment at: test/Headers/Inputs/include/cstdlib:2
+#pragma once
+typedef __SIZE_TYPE__ size_t;

Where is this used? Are there tests missing?


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea updated this revision to Diff 199041.
gtbercea added a comment.

- Fix cstdlib issue.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765

Files:
  lib/Driver/ToolChains/Clang.cpp
  lib/Headers/CMakeLists.txt
  lib/Headers/__clang_cuda_math_forward_declares.h
  lib/Headers/openmp_wrappers/__clang_openmp_math.h
  lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  lib/Headers/openmp_wrappers/cmath
  lib/Headers/openmp_wrappers/math.h
  test/Headers/Inputs/include/cstdlib
  test/Headers/nvptx_device_cmath_functions.c
  test/Headers/nvptx_device_cmath_functions.cpp
  test/Headers/nvptx_device_math_functions.c
  test/Headers/nvptx_device_math_functions.cpp

Index: test/Headers/nvptx_device_math_functions.cpp
===
--- test/Headers/nvptx_device_math_functions.cpp
+++ test/Headers/nvptx_device_math_functions.cpp
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -include cstdlib -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: test/Headers/nvptx_device_math_functions.c
===
--- test/Headers/nvptx_device_math_functions.c
+++ test/Headers/nvptx_device_math_functions.c
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: test/Headers/nvptx_device_cmath_functions.cpp
===
--- test/Headers/nvptx_device_cmath_functions.cpp
+++ test/Headers/nvptx_device_cmath_functions.cpp
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include cmath -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: 

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

This always includes the declare file but not the define file, correct?

Could we have 4 tests that are compiled in target mode with:

  // with and without math.h/cmath (clang/clang++)
  #include 
  
  long abs(long __i) { return (__i < 0 ? -i : i); }


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea updated this revision to Diff 198929.
gtbercea added a comment.

- Remove define.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61765

Files:
  lib/Driver/ToolChains/Clang.cpp
  lib/Headers/CMakeLists.txt
  lib/Headers/openmp_wrappers/__clang_openmp_math.h
  lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  lib/Headers/openmp_wrappers/cmath
  lib/Headers/openmp_wrappers/math.h
  test/Headers/nvptx_device_cmath_functions.c
  test/Headers/nvptx_device_cmath_functions.cpp
  test/Headers/nvptx_device_math_functions.c
  test/Headers/nvptx_device_math_functions.cpp

Index: test/Headers/nvptx_device_math_functions.cpp
===
--- test/Headers/nvptx_device_math_functions.cpp
+++ test/Headers/nvptx_device_math_functions.cpp
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: test/Headers/nvptx_device_math_functions.c
===
--- test/Headers/nvptx_device_math_functions.c
+++ test/Headers/nvptx_device_math_functions.c
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: test/Headers/nvptx_device_cmath_functions.cpp
===
--- test/Headers/nvptx_device_cmath_functions.cpp
+++ test/Headers/nvptx_device_cmath_functions.cpp
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include cmath -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: test/Headers/nvptx_device_cmath_functions.c
===
--- 

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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea created this revision.
gtbercea added reviewers: jdoerfert, ABataev, hfinkel, caomhin.
Herald added subscribers: cfe-commits, guansong, mgorny.
Herald added a project: clang.

This patches fixes an issue in which the __clang_cuda_cmath.h header is being 
included even when cmath or math.h headers are not included.


Repository:
  rC Clang

https://reviews.llvm.org/D61765

Files:
  lib/Driver/ToolChains/Clang.cpp
  lib/Headers/CMakeLists.txt
  lib/Headers/openmp_wrappers/__clang_openmp_math.h
  lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  lib/Headers/openmp_wrappers/cmath
  lib/Headers/openmp_wrappers/math.h
  test/Headers/nvptx_device_cmath_functions.c
  test/Headers/nvptx_device_cmath_functions.cpp
  test/Headers/nvptx_device_math_functions.c
  test/Headers/nvptx_device_math_functions.cpp

Index: test/Headers/nvptx_device_math_functions.cpp
===
--- test/Headers/nvptx_device_math_functions.cpp
+++ test/Headers/nvptx_device_math_functions.cpp
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -internal-isystem %S/Inputs/include -include stdlib.h -include limits -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: test/Headers/nvptx_device_math_functions.c
===
--- test/Headers/nvptx_device_math_functions.c
+++ test/Headers/nvptx_device_math_functions.c
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include math.h -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include math.h -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
 
 #include 
 
Index: test/Headers/nvptx_device_cmath_functions.cpp
===
--- test/Headers/nvptx_device_cmath_functions.cpp
+++ test/Headers/nvptx_device_cmath_functions.cpp
@@ -4,7 +4,7 @@
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -internal-isystem %S/Inputs/include -include cmath -x c++ -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
-// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck -check-prefix CHECK-YES %s
+// RUN: %clang_cc1 -internal-isystem %S/../../lib/Headers/openmp_wrappers -include __clang_openmp_math_declares.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -include cmath -internal-isystem %S/Inputs/include -include stdlib.h -x c++ -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck