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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.

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

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

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

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

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

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:

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

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

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

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:

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

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

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

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