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

Wed May 3 11:34:14 PDT 2023

mstorsjo added a comment.

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

> In D149726#4315736 <https://reviews.llvm.org/D149726#4315736>, @mstorsjo wrote:
>
>> But overall this patch doesn’t change anything of that; if C++17 is supported, such a compiler flag is enabled, if not, the compiler default is used - this is the same behavior as before. The only difference is that it is done in a way that behaves consistently across compilers.
>
> IIUC, if the compiler doesn't support C++17, `libomptarget` will still be enabled with this patch (assuming all the other conditions are met).

Right, that’s true. Initially, when `libomptarget` was added, the extra criterion for it only specified C++11, and then it had been increased to 14 and 17, but I somewhat assumed that the extra modern C++ code used there might be only C++11 anyway. But you’re right that it could just as well have started taking advantage of  C++17 features too.

I’ll see if we can use the cmake language version selection, while still conditionally enabling it only if a new enough version is available.

>>> 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.
>>
>> Sure, that might be for the best. But that patch has been in the tree for almost two months already, and if nobody else has run into issue with it, apparently it’s not an issue in practice?
>
> That's not necessary the case. Most `libomp` users only use release version.

Right. In any case, the same issue seems to have happened for `libomp` here; since it by default is built in C++17 mode if that is supported, and most builders of the git version support it, we might have implicitly started requiring C++17.

Perhaps the reverse would be best for `libomp` - don’t select C++17 but stick to C++03 explicitly, so 
all users build it with the same toolchain options, so we don’t accidentally start requiring C++17 features?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149726