These are needed by both libraries, so we can do that in a
common namespace and unify configuration parameters.
Also make sure that the user isn't requesting libomptarget
if the library cannot be built on the system. Issue an error
in that case.
Details
Details
Diff Detail
Diff Detail
Event Timeline
| libomptarget/CMakeLists.txt | ||
|---|---|---|
| 46–47 | While we are cleaning up... I think this would be cleaner if all prerequisites were checked here and an error issued if one is not met. Currently there seems to be only one (need -std=c++11 flag) I don't think it would hurt to check for users trying to build on mac or windows here either and issue a similar message, because that would mean they tried to improperly override OPENMP_ENABLE_LIBOMPTARGET # Check prerequisites for building libomptarget
if (NOT OPENMP_HAVE_STD_CPP11_FLAG)
libomptarget_error_say("Requested build of libomptarget with no -std=c++11 flag")
endif()You could also add OPENMP_HAVE_STD_CPP11_FLAG into the logic which determines the default ENABLE_LIBOMPTARGET inside the top-level CMakeLists.txt file. If a user somehow finds them self inside the libomptarget/CMakeLists.txt file without -std=c++11 flag support. I think its better for CMake to scream at the user during config time rather than for a status message to get lost in a sea of CMake output, because then the user builds all of LLVM and they scratch their heads trying to find libomptarget. | |
| libomptarget/CMakeLists.txt | ||
|---|---|---|
| 46–47 | I disagree: If the user explicitly tells the build system to build libomptarget, it's the user's fault. Regarding C++11: Doesn't libomp also depend on that standard? Maybe we should just error out completely in the top-level CMakeLists.txt, I think LLVM requires a C++11-capable compiler anyway... | |
| libomptarget/CMakeLists.txt | ||
|---|---|---|
| 46–47 | Actually we are in total agreement that the user is at fault. But, one of the main goals of a configuration system is to check prerequisites and then error out when they aren't met. I'm in favor of aiding the user in seeing the error of their ways (or setup). At the *very least*, it should be a warning so that the output is pronounced in the CMake output. It does not make sense to me to require something for the build, but not error out when you do not have the prerequisites but at the same time request the feature (either by the default, ENABLE_LIBOMPTARGET, which currently does not check for the flag, or explicitly setting OPENMP_ENABLE_LIBOMPTARGET=On). To me, this boils down to when OPENMP_ENABLE_LIBOMPTARGET=On for whatever reason, then it implies the user absolutely wants the library. There should be checks for attribute((weak)) as well. We can't error out in the top level because libomp is supported on Visual Studio which doesn't use the -std=c++11. | |
| libomptarget/CMakeLists.txt | ||
|---|---|---|
| 46–47 | Ok, then I'd say we just shouldn't make OPENMP_ENABLE_LIBOMPTARGET available on Windows and Mac OS X in the top-level CMakeLists.txt. Because it's not reasonable to have a switch that the user isn't allowed to use because the build system will yell immediately. I still think that we should silently accept if a compiler doesn't support C++11: The build system builds whatever it finds possible with the given compiler. Maybe we should switch it to a warning though? | |
Comment Actions
I'm going to make one last plea for doing it correctly. Here is actual CMake code from LLVM's main CMakeLists.txt. It is reasonable to error-check what the user requests no matter how odd you may find it! The user may not know that something is only available on a certain platform until they request it. They do not wrap the option in operating system guards. Although some other options they do wrap in OS guards. Please just add the three line OS error check and the three line flag check.
option(LLVM_USE_INTEL_JITEVENTS
"Use Intel JIT API to inform Intel(R) VTune(TM) Amplifier XE 2011 about JIT code"
OFF)
if( LLVM_USE_INTEL_JITEVENTS )
# Verify we are on a supported platform
if( NOT CMAKE_SYSTEM_NAME MATCHES "Windows" AND NOT CMAKE_SYSTEM_NAME MATCHES "Linux" )
message(FATAL_ERROR
"Intel JIT API support is available on Linux and Windows only.")
endif()
endif( LLVM_USE_INTEL_JITEVENTS )
option(LLVM_USE_OPROFILE
"Use opagent JIT interface to inform OProfile about JIT code" OFF)
# If enabled, verify we are on a platform that supports oprofile.
if( LLVM_USE_OPROFILE )
if( NOT CMAKE_SYSTEM_NAME MATCHES "Linux" )
message(FATAL_ERROR "OProfile support is available on Linux only.")
endif( NOT CMAKE_SYSTEM_NAME MATCHES "Linux" )
endif( LLVM_USE_OPROFILE )Also, here is LLVM's -std=c++11 flag check, so you could use this if you want (notice the FATAL_ERROR):
if (LLVM_ENABLE_CXX1Y)
check_cxx_compiler_flag("-std=c++1y" CXX_SUPPORTS_CXX1Y)
append_if(CXX_SUPPORTS_CXX1Y "-std=c++1y" CMAKE_CXX_FLAGS)
elseif(LLVM_ENABLE_CXX1Z)
check_cxx_compiler_flag("-std=c++1z" CXX_SUPPORTS_CXX1Z)
append_if(CXX_SUPPORTS_CXX1Z "-std=c++1z" CMAKE_CXX_FLAGS)
else()
check_cxx_compiler_flag("-std=c++11" CXX_SUPPORTS_CXX11)
if (CXX_SUPPORTS_CXX11)
if (CYGWIN OR MINGW)
# MinGW and Cygwin are a bit stricter and lack things like
# 'strdup', 'stricmp', etc in c++11 mode.
append("-std=gnu++11" CMAKE_CXX_FLAGS)
else()
append("-std=c++11" CMAKE_CXX_FLAGS)
endif()
else()
message(FATAL_ERROR "LLVM requires C++11 support but the '-std=c++11' flag isn't supported.")
endif()
endif()Comment Actions
Ok, I finally got your point. I especially agree on "The user may not know that something is only available on a certain platform until they request it" which kind of invalidates my previous arguments. I will update the patch tomorrow. I'm still tending towards putting this into the top-level CMakeLists.txt, right next to the definition of OPENMP_ENABLE_LIBOMPTARGET to have it in one place. What do you think?
Regarding -std=c++11: How does MSVC survive that LLVM check?
While we are cleaning up... I think this would be cleaner if all prerequisites were checked here and an error issued if one is not met. Currently there seems to be only one (need -std=c++11 flag) I don't think it would hurt to check for users trying to build on mac or windows here either and issue a similar message, because that would mean they tried to improperly override OPENMP_ENABLE_LIBOMPTARGET
# Check prerequisites for building libomptarget if (NOT OPENMP_HAVE_STD_CPP11_FLAG) libomptarget_error_say("Requested build of libomptarget with no -std=c++11 flag") endif()You could also add OPENMP_HAVE_STD_CPP11_FLAG into the logic which determines the default ENABLE_LIBOMPTARGET inside the top-level CMakeLists.txt file.
If a user somehow finds them self inside the libomptarget/CMakeLists.txt file without -std=c++11 flag support. I think its better for CMake to scream at the user during config time rather than for a status message to get lost in a sea of CMake output, because then the user builds all of LLVM and they scratch their heads trying to find libomptarget.