[OpenMP] Use CMAKE_CXX_STANDARD for setting the C++ version

[mstorsjo]

Summary

Previously, we tried to check whether the -std=c++17 option was
supported and manually add the flag. That doesn't work for compilers
that do support C++17 but use a different option syntax, like
clang-cl.

OpenMP itself probably doesn't specifically require C++17, therefore
CXX_STANDARD_REQUIRED is left off, but in some cases, we may
have code that only works in C++17 mode.

In particular, 46262cab24312c71717ca70a9d0700481aa59152 made a
refactoring that works when built with Clang in C++17 mode, but not
in C++14 mode. MSVC accepts the construct in both language modes.

For libomptarget, we've had specific checks that require C++17
(or the -std=c++17 option) to be supported. It's doubtful that
libomptarget has got any code which more specifically requires C++17;
this seems to be a remnant from when libomptarget was added
originally in 2467df6e4f04e3d0e8e78d662473ba1b87c0a885 / D14031.
At that point, the rest of OpenMP didn't require C++11, while
libomptarget did require it. Now, it's unlikely that anyone attempts
building it with a toolchain that doesn't support C++11.

At this point, we could also probably just set CXX_STANDARD_REQUIRED
to true, requiring C++17 as baseline for all the OpenMP libraries.

This fixes building OpenMP with clang-cl after
46262cab24312c71717ca70a9d0700481aa59152.

[hans]

I don't know anything about openmp's build, but I'm very happy if this fixes it, so +1 from me :)

[tianshilei1992]

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.

[mstorsjo]

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

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.

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?

In any case, I don’t have much of an opinion on that and don’t mind reverting it, but the fact that openmp is built in C++17 mode in some configs but not others, even where C++17 is supported, is a bit inconsistent.

[tianshilei1992]

In D149726#4315736, @mstorsjo wrote:

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

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

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.

[mstorsjo]

In D149726#4315926, @tianshilei1992 wrote:

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

[ye-luo]

openmp/CMakeLists.txt

49

Probably also need CMAKE_CXX_EXTENSIONS=OFF to have exactly -std=c++17, otherwise gnu extension might be on.

[mstorsjo]

Retain the C++17 checks for libomptarget, but done by checking "cxx_std_17" IN_LIST CMAKE_CXX_COMPILE_FEATURES instead of checking OPENMP_HAVE_STD_CPP17_FLAG.

Setting CMAKE_CXX_EXTENSIONS explicitly to NO too.

If the main libomp is supposed to be buildable with compilers that don't even support C++11, then it would probably be best in the long run to start setting CMAKE_CXX_STANDARD explicitly to the minimum level, so that all build configurations build in the same language mode - but I'd leave that up for others as a separate change.

Now this should be a no-op change for compiler that do use the -std=c++17 syntax, while making things consistent with those others, for compilers with a different argument syntax.

[mstorsjo]

Ping @tianshilei1992 - does this version address your concerns?

In any case, I don't mind if you revert 46262cab24312c71717ca70a9d0700481aa59152 either, since that might break building libomp in older C++ versions.

[MaskRay]

In 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 (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" situation.

[tianshilei1992]

In D149726#4336554, @MaskRay wrote:

In 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 (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" 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).

[tianshilei1992]

LGTM now.

[MaskRay]

In D149726#4336912, @tianshilei1992 wrote:

In D149726#4336554, @MaskRay wrote:

In 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 (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" 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...

[tianshilei1992]

@MaskRay @mstorsjo We make a consensus in the community that both libomp and libomptarget will follow the same C++ requirement as LLVM, though for libomp it is still supposed to be a C library. Now we can set the CXX standard version globally for OpenMP project.

[MaskRay]

In D149726#4384137, @tianshilei1992 wrote:

@MaskRay @mstorsjo We make a consensus in the community that both libomp and libomptarget will follow the same C++ requirement as LLVM, though for libomp it is still supposed to be a C library. Now we can set the CXX standard version globally for OpenMP project.

Thanks for the heads-up and raising this issue among OpenMP folks. It's good to know that OpenMP is no longer an exception now!

openmp/CMakeLists.txt

if (OPENMP_STANDALONE_BUILD)
   set(OPENMP_INSTALL_LIBDIR "lib${OPENMP_LIBDIR_SUFFIX}")
 
   # Group test settings.
   set(OPENMP_TEST_C_COMPILER ${CMAKE_C_COMPILER} CACHE STRING
     "C compiler to use for testing OpenMP runtime libraries.")
   set(OPENMP_TEST_CXX_COMPILER ${CMAKE_CXX_COMPILER} CACHE STRING
     "C++ compiler to use for testing OpenMP runtime libraries.")
   set(OPENMP_LLVM_TOOLS_DIR "" CACHE PATH "Path to LLVM tools for testing.")
+
+  set(CMAKE_CXX_STANDARD 17 CACHE STRING "C++ standard to conform to")
+  set(CMAKE_CXX_STANDARD_REQUIRED NO)
 else()
   set(OPENMP_ENABLE_WERROR ${LLVM_ENABLE_WERROR})
   # If building in tree, we honor the same install suffix LLVM uses.
   set(OPENMP_INSTALL_LIBDIR "lib${LLVM_LIBDIR_SUFFIX}")
 
   if (NOT MSVC)
     set(OPENMP_TEST_C_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang)
     set(OPENMP_TEST_CXX_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang++)
   else()
     set(OPENMP_TEST_C_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang.exe)
     set(OPENMP_TEST_CXX_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang++.exe)
   endif()
+
+  # If not standalone, set CMAKE_CXX_STANDARD but don't set the global cache value,
+  # only set it locally for OpenMP.
+  set(CMAKE_CXX_STANDARD 17)
+  set(CMAKE_CXX_STANDARD_REQUIRED NO)
 endif()
 
 # Check and set up common compiler flags.
 include(config-ix)
 include(HandleOpenMPOptions)
 
 # Set up testing infrastructure.
 include(OpenMPTesting)
 
 set(OPENMP_TEST_FLAGS "" CACHE STRING
   "Extra compiler flags to send to the test compiler.")
 set(OPENMP_TEST_OPENMP_FLAGS ${OPENMP_TEST_COMPILER_OPENMP_FLAGS} CACHE STRING
   "OpenMP compiler flag to use for testing OpenMP runtime libraries.")
 
 set(ENABLE_LIBOMPTARGET ON)
 # Currently libomptarget cannot be compiled on Windows or MacOS X.
 # Since the device plugins are only supported on Linux anyway,
 # there is no point in trying to compile libomptarget on other OSes.
 # 32-bit systems are not supported either.
-if (APPLE OR WIN32 OR NOT OPENMP_HAVE_STD_CPP17_FLAG OR NOT CMAKE_SIZEOF_VOID_P EQUAL 8)
+if (APPLE OR WIN32 OR NOT CMAKE_SIZEOF_VOID_P EQUAL 8)
   set(ENABLE_LIBOMPTARGET OFF)
 endif()
 
 option(OPENMP_ENABLE_LIBOMPTARGET "Enable building libomptarget for offloading."
        ${ENABLE_LIBOMPTARGET})
 option(OPENMP_ENABLE_LIBOMP_PROFILING "Enable time profiling for libomp." OFF)
 
 # Header install location
 if(${OPENMP_STANDALONE_BUILD})
   set(LIBOMP_HEADERS_INSTALL_PATH "${CMAKE_INSTALL_INCLUDEDIR}")
 else()
   string(REGEX MATCH "[0-9]+" CLANG_VERSION ${PACKAGE_VERSION})
   set(LIBOMP_HEADERS_INSTALL_PATH "${OPENMP_INSTALL_LIBDIR}/clang/${CLANG_VERSION}/include")
 endif()
 
 # Build host runtime library, after LIBOMPTARGET variables are set since they are needed
 # to enable time profiling support in the OpenMP runtime.
 add_subdirectory(runtime)
 
 if (OPENMP_ENABLE_LIBOMPTARGET)
   # Check that the library can actually be built.
   if (APPLE OR WIN32)
     message(FATAL_ERROR "libomptarget cannot be built on Windows and MacOS X!")
-  elseif (NOT OPENMP_HAVE_STD_CPP17_FLAG)
-    message(FATAL_ERROR "Host compiler must support C++17 to build libomptarget!")
   elseif (NOT CMAKE_SIZEOF_VOID_P EQUAL 8)
     message(FATAL_ERROR "libomptarget on 32-bit systems are not supported!")
   endif()
 
   add_subdirectory(libomptarget)
 endif()
 
 set(ENABLE_OMPT_TOOLS ON)

openmp/cmake/HandleOpenMPOptions.cmake

 # being unrecognized only if the compilation triggers other warnings to be
 # printed. Therefore, check for whether the compiler supports options in the
 # form -W<foo>, and if supported, add the corresponding -Wno-<foo> option.
 
 append_if(OPENMP_HAVE_WENUM_CONSTEXPR_CONVERSION_FLAG "-Wno-enum-constexpr-conversion" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 append_if(OPENMP_HAVE_WEXTRA_FLAG "-Wno-extra" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 append_if(OPENMP_HAVE_WPEDANTIC_FLAG "-Wno-pedantic" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 append_if(OPENMP_HAVE_WMAYBE_UNINITIALIZED_FLAG "-Wno-maybe-uninitialized" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
-
-append_if(OPENMP_HAVE_STD_CPP17_FLAG "-std=c++17" CMAKE_CXX_FLAGS)

openmp/cmake/config-ix.cmake

 # being unrecognized only if the compilation triggers other warnings to be
 # printed. Therefore, check for whether the compiler supports options in the
 # form -W<foo>, and if supported, add the corresponding -Wno-<foo> option.
 
 check_cxx_compiler_flag(-Wenum-constexpr-conversion OPENMP_HAVE_WENUM_CONSTEXPR_CONVERSION_FLAG)
 check_cxx_compiler_flag(-Wextra OPENMP_HAVE_WEXTRA_FLAG)
 check_cxx_compiler_flag(-Wpedantic OPENMP_HAVE_WPEDANTIC_FLAG)
 check_cxx_compiler_flag(-Wmaybe-uninitialized OPENMP_HAVE_WMAYBE_UNINITIALIZED_FLAG)
-
-check_cxx_compiler_flag(-std=c++17 OPENMP_HAVE_STD_CPP17_FLAG)