In OpenMP device offloading we must ensure that unde C++ 17, the inclusion of cstdlib will works correctly.
Details
- Reviewers
ABataev tra jdoerfert hfinkel caomhin - Commits
- rZORGc1f861d8515c: [OpenMP][bugfix] Fix issues with C++ 17 compilation when handling math functions
rGc1f861d8515c: [OpenMP][bugfix] Fix issues with C++ 17 compilation when handling math functions
rG7641f310d7b0: [OpenMP][bugfix] Fix issues with C++ 17 compilation when handling math functions
rL360804: [OpenMP][bugfix] Fix issues with C++ 17 compilation when handling math functions
rC360804: [OpenMP][bugfix] Fix issues with C++ 17 compilation when handling math functions
Diff Detail
- Repository
- rC Clang
Event Timeline
lib/Headers/__clang_cuda_device_functions.h | ||
---|---|---|
1486 | If I recall correctly, __cplusplus is not defined in C mode, so both GCC and Clang will issue a warning with -Wundef. Maybe this can be solved with something similar to: #ifdef __cplusplus #define cpp_version __cplusplus #else #define cpp_version 0 #endif (to avoid repetition of #if defined(__cplusplus) && __cplusplus >= 201703L) |
Missing defined(__cplusplus) to avoid warnings and we have to resolve the two new includes. Otherwise, this looks good. So, if you "fix" both go ahead and commit, if not, lets discuss.
lib/Headers/__clang_cuda_device_functions.h | ||
---|---|---|
1486 | I dislike defining the version and advice to repeating #if defined(__cplusplus) && __cplusplus >= 201703L) | |
lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h | ||
24 ↗ | (On Diff #199613) | I ask this question again and again, do we need them now? Why? |
lib/Headers/__clang_cuda_device_functions.h | ||
---|---|---|
1486 | I agree with @jdoerfert. | |
1584–1591 | This is typically dealt with by defining and using a macro which would expand to noexcept or to nothing. This should reduce the number of needed #if. |
LGTM.
lib/Headers/__clang_cuda_cmath.h | ||
---|---|---|
42 | I think the change is useful for CUDA, too, but I'm OK keeping it OpenMP-specific for now. I can enable it for CUDA later. |
lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h | ||
---|---|---|
24 ↗ | (On Diff #199613) | Why did you add these includes? |
I think the change is useful for CUDA, too, but I'm OK keeping it OpenMP-specific for now. I can enable it for CUDA later.