[Openmp-commits] [PATCH] D149726: [OpenMP] Use CMAKE_CXX_STANDARD for setting the C++ version

Fri May 12 20:22:09 PDT 2023

MaskRay added a comment.

In D149726#4336912 <https://reviews.llvm.org/D149726#4336912>, @tianshilei1992 wrote:

> In D149726#4336554 <https://reviews.llvm.org/D149726#4336554>, @MaskRay wrote:
>
>> In D149726#4315555 <https://reviews.llvm.org/D149726#4315555>, @tianshilei1992 wrote:
>>
>>> `libomp` is widely used and it only uses a small set of C++ features. Due to some historical issue that `libomptarget` and `libomp` are in the same `openmp` directory but their developments are quite diverged. I can see requiring C++17 for `libomp` will definitely break a lot user's build. That will need to be further discussed in our weekly multi-company meeting. The patch mentioned in the description didn't consider the special situation we have for `libomp`. If necessary, we might revert it in the future.
>>
>> I don't understand this statement that requiring C++17 for `libomp` will definitely break a lot of users builds.
>> Isn't that libomp is mostly built in C++17 modes, except in the `OPENMP_STANDALONE_BUILD` mode on some Windows configurations due to unrecognized `-std=c++17`?
>>
>> The `-std=c++17` occurrences in existing openmp/ files well suggest that requiring C++17 is intended (af28b27d31a5c13c31769c8551ffdcdc469fd5f4 <https://reviews.llvm.org/rGaf28b27d31a5c13c31769c8551ffdcdc469fd5f4> (2022-08)).
>>
>> Please be more specific what other configurations you'd like to support. If you want more builds supported I'd like to see a proposal for OpenMP like https://discourse.llvm.org/t/rfc-stand-alone-build-support/61291 (RFC: Stand-alone build support).
>> Having no build bot for the configuration you care about and making a statement "it will break a lot of users builds" just creates a "haunted graveyard" <https://www.usenix.org/sites/default/files/conference/protected-files/srecon17americas_slides_reese.pdf> situation.
>
> First, if you take a close look at the patch you pointed, it only checks the existence of the flag, but not enforces it. What's more, it is more for `libomptarget` instead of `libomp`, though we did set it for LIT tests.
> Second, like I mentioned before, the development of `libomp` and `libomptarget` are diverged. We follow roughly the same standard as LLVM for `libomptarget`, but for `libomp` we choose to be much more conservative. Many HPC users only use `libomp`, but due to the complication of those HPC systems (for example, there are large number of HPC users still using GCC 4.8.5), enforcing C++17 globally will definitely break their build. I'm not against the change, but we have to discuss further in our meeting, especially with Intel folks since `libomp` was contributed by them. I'll bring this to the next meeting.
> As for build bot, we don't have build bot for `libomp` at all (we used to).

Thanks. I'll need to learn more about libomp and libomptarget. The ATOMIC_VAR_INIT change isn't really C++17 compatible.
There is likely a bug in clang-cl that is incompatible with MSVC's header files `<atomic>`.

As a lazy resort, we can say that libomp requires C++ 17 if clang-cl is used, but a more relaxed requirement with GCC...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149726