Page MenuHomePhabricator

[libomptarget] Require LLVM source tree to build libomptarget
ClosedPublic

Authored by JonChesterfield on Oct 14 2020, 2:26 PM.

Details

Summary

[libomptarget] Require LLVM source tree to build libomptarget

This is to permit reliably #including files from the LLVM tree in libomptarget,
as an improvement on the copy and paste that is currently in use. See D87841
for the first example of removing duplication given this new requirement.

The weekly openmp dev call reached consensus on this approach. See also D87841
for some alternatives that were considered. In the future, we may want to
introduce a new top level repo for shared constants, or start using the ADT
library within openmp.

This will break sufficiently exotic build systems, trivial fixes as below.

Building libomptarget as part of the monorepo will continue to work.
If openmp is built separately, it now requires a cmake macro indicating
where to find the LLVM source tree.

If openmp is built separately, without the llvm source tree already on disk,
the build machine will need a copy of a subset of the llvm source tree and
the cmake macro indicating where it is.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2020, 2:26 PM
JonChesterfield requested review of this revision.Oct 14 2020, 2:26 PM

Will it still be possible to build the OpenMP runtime without LLVM headers/sources? Can we disable libomptarget if LLVM headers are not available?

This test is in libomptarget, so that should work. I don't know if there's an existing hook in libomp to disable libomptarget, or whether the former actually works without the latter. Can take a look.

It is a FATAL_ERROR so when CMake decides to include libomptarget, this would break the CMake generation.

In openmp/CMakeLists.txt there is ENABLE_LIBOMPTARGET to decide about the default value for the CMake option OPENMP_ENABLE_LIBOMPTARGET. This way libomptarget gets disabled on apple and win32 by default.

if (OPENMP_STANDALONE_BUILD and NOT LLVM_MAIN_INCLUDE_DIR)
  set(ENABLE_LIBOMPTARGET OFF)
endif()

cmake -DOPENMP_ENABLE_LIBOMPTARGET=ON would allow to overwrite this default.

I'm unclear on what that cmake is doing. Copied here:

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.                                                                                                                                               
if (APPLE OR WIN32 OR NOT OPENMP_HAVE_STD_CPP14_FLAG)
  set(ENABLE_LIBOMPTARGET OFF)
endif()

option(OPENMP_ENABLE_LIBOMPTARGET "Enable building libomptarget for offloading."
       ${ENABLE_LIBOMPTARGET})
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_CPP14_FLAG)
    message(FATAL_ERROR "Host compiler must support C++14 to build libomptarget!")
  endif()

  add_subdirectory(libomptarget)
endif()

Is this using ENABLE_LIBOMPTARGET as the default value for OPENMP_ENABLE_LIBOMPTARGET? If so, does setting OPENMP_ENABLE_LIBOMPTARGET win out?

A reasonable guess seems to be:

if (NOT LLVM_MAIN_INCLUDE_DIR)
message(...) #not fatal
set(ENABLE_LIBOMPTARGET OFF)
endif()

Probably combined with the above for anyone who builds libomptarget without the containing libomp.

Will it still be possible to build the OpenMP runtime without LLVM headers/sources? Can we disable libomptarget if LLVM headers are not available?

FWIW, that was (my) intend.

  • review comments

Answering my own questions about cmake semantics by trial and error, the above revision handles the missing definition by not building libomptarget.

Tested the various permutations of LLVM_MAIN_INCLUDE_DIR present/absent in each file locally and all behave as one would hope.

Passing -DOPENMP_ENABLE_LIBOMPTARGET wins out over this default. Picked 'status' as that is used with message elsewhere in openmp's cmake.

ENABLE_LIBOMPTARGET determines the initial default value for OPENMP_ENABLE_LIBOMPTARGET. Therefore the variable is only relevant in the first cmake run, but has no effect in successive cmake or ccmake configures. It will never overwrite the value of OPENMP_ENABLE_LIBOMPTARGET.

I'm not sure, whether the mono-repo structure is considered somewhere to find LLVM_MAIN_INCLUDE_DIR, but we should set the variable here to allow building openmp stand-alone in a mono-repo clone.

openmp/CMakeLists.txt
69–72

This change would allow to build openmp (with libomptarget) standalone, while checked out as mono-repo:

set(LLVM_MAIN_INCLUDE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/../llvm/include) is interesting.

I'm not sure it's reasonable for openmp to guess at a plausible definition for LLVM_MAIN_INCLUDE_DIR and overwrite it. That composes poorly with (possibly imaginary) cmake that do their own testing based on this variable.

How about we introduce a whole new one,
LIBOMPTARGET_LLVM_MAIN_INCLUDE_DIR,
which is set to LLVM_MAIN_INCLUDE_DIR if available, otherwise to that guess, otherwise skip libomptarget?

  • review comments
  • Try harder to find LLVM on disk

New spin. Attempts to handle LLVM_MAIN_INCLUDE_DIR missing by looking for a plausible source directory, and avoids setting a variable which isn't LIBOMPTARGET_ prefixed.

This revision is now accepted and ready to land.Oct 21 2020, 8:33 AM