Page MenuHomePhabricator

[CMake] Refactor common settings and flags
ClosedPublic

Authored by Hahnfeld on Nov 15 2017, 8:13 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

Hahnfeld created this revision.Nov 15 2017, 8:13 AM
Hahnfeld updated this revision to Diff 123996.Nov 22 2017, 11:44 AM

Rebase onto recent changes.

Hahnfeld updated this revision to Diff 123997.Nov 22 2017, 11:50 AM

Remove project() that crept back into libomptarget/CMakeLists.txt.

jlpeyton added inline comments.Nov 28 2017, 10:44 AM
libomptarget/CMakeLists.txt
47–49 ↗(On Diff #123997)

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.

Hahnfeld added inline comments.Nov 28 2017, 10:51 AM
libomptarget/CMakeLists.txt
47–49 ↗(On Diff #123997)

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

jlpeyton added inline comments.Nov 28 2017, 1:04 PM
libomptarget/CMakeLists.txt
47–49 ↗(On Diff #123997)

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.

Hahnfeld added inline comments.Nov 28 2017, 1:13 PM
libomptarget/CMakeLists.txt
47–49 ↗(On Diff #123997)

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?

jlpeyton edited edge metadata.Nov 28 2017, 2:33 PM

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

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?

Hahnfeld updated this revision to Diff 124743.Nov 29 2017, 7:03 AM
Hahnfeld edited the summary of this revision. (Show Details)

Check that the user isn't requesting libomptarget on unsupported configurations.

Hahnfeld marked 4 inline comments as done.Nov 29 2017, 7:04 AM
This revision is now accepted and ready to land.Nov 29 2017, 9:24 AM
This revision was automatically updated to reflect the committed changes.