[Libomptarget] Make libomptarget an LLVM library

[jhuber6]

Summary

This patch makes libomptarget depend on LLVM libraries to be built. The
reason for this is because we already have an implicit dependency on
LLVM headers for ELF identification and extraction as well as an
optional dependenly on the LLVMSupport library for time tracing
information. Furthermore, there are changes in the future that require
using more LLVM libraries, and will heavily simplify some future code as
well as open up the large amount of useful LLVM libraries to
libomptarget.

This will make "standalone" builds of `libomptarget' more difficult for
vendors wishing to ship their own. This will require a sufficiently new
version of LLVM to be installed on the system that should be picked up
by the existing handling for the implicit headers.

The things this patch changes are as follows:

• libomptarget.so links against LLVMSupport and LLVMObject
• libomptarget.so is a symbolic link to libomptarget.so.15
• If using a shared library build, user applications will depend on LLVM libraries as well
• We can now use LLVM resources in Libomptarget.

Note that this patch only changes this to apply to libomptarget itself,
not the plugins. Additional patches will be necessary for that.

[JonChesterfield]

It's going to be difficult to tell which vendors or CI systems are affected without landing this. I remember objections to that strategy in the past, anyone have an alternative suggestion?

[jhuber6]

Change link libraries to LINK_LIBS.

[jhuber6]

In D129875#3656358, @JonChesterfield wrote:

It's going to be difficult to tell which vendors or CI systems are affected without landing this. I remember objections to that strategy in the past, anyone have an alternative suggestion?

I didn't have any problems locally doing either a LLVM_ENABLE_PROJECTS=openmp or a LLVM_ENABLE_RUNTIMES=openmp build. However it seems the pre-merge checks end up with a lot of failures. It may just require rebuilding properly.

Another advantage of this is that we should be able to drop the dl, libelf, and libffi dependencies and use some LLVM libraries instead.

[ye-luo]

Prefer additional dependency remains optional.

[JonChesterfield]

In D129875#3656403, @ye-luo wrote:

Prefer additional dependency remains optional.

That's the problem really. It means reimplementing a bunch of object format handling and sticking with raw dlopen etc. The return on the investment of DIY or copy/pasting blocks of llvm source is really poor relative to requiring llvm to build the library from llvm.

[jhuber6]

In D129875#3656403, @ye-luo wrote:

Prefer additional dependency remains optional.

The only downsides I see from this approach is that user code will depend on LLVM libraries if LLVM is build using shared libraries. But I'm pretty sure the default build method in LLVM is to use static libraries so that shouldn't propagate to users just wishing to build libomptarget alone. We may have some vendors complain about the extra dependency, but I'm personally not aware of anyone using Clang/LLVM for OpenMP offloading that isn't building it with LLVM or getting it through a release that was build with LLVM.

[ye-luo]

In D129875#3656431, @jhuber6 wrote:

In D129875#3656403, @ye-luo wrote:

Prefer additional dependency remains optional.

The only downsides I see from this approach is that user code will depend on LLVM libraries if LLVM is build using shared libraries. But I'm pretty sure the default build method in LLVM is to use static libraries so that shouldn't propagate to users just wishing to build libomptarget alone. We may have some vendors complain about the extra dependency, but I'm personally not aware of anyone using Clang/LLVM for OpenMP offloading that isn't building it with LLVM or getting it through a release that was build with LLVM.

That is exactly my concern. Won't be happy to see llvm libraries on the application side. If it is internal to the libomptarget, that is fine. It seems all the llvm libraries are static in my builds. Then it is likely OK.

[jhuber6]

In D129875#3656522, @ye-luo wrote:

In D129875#3656431, @jhuber6 wrote:

In D129875#3656403, @ye-luo wrote:

Prefer additional dependency remains optional.

The only downsides I see from this approach is that user code will depend on LLVM libraries if LLVM is build using shared libraries. But I'm pretty sure the default build method in LLVM is to use static libraries so that shouldn't propagate to users just wishing to build libomptarget alone. We may have some vendors complain about the extra dependency, but I'm personally not aware of anyone using Clang/LLVM for OpenMP offloading that isn't building it with LLVM or getting it through a release that was build with LLVM.

That is exactly my concern. Won't be happy to see llvm libraries on the application side. If it is internal to the libomptarget, that is fine. It seems all the llvm libraries are static in my builds. Then it is likely OK.

If you want to make extra sure you can apply this patch and use the new clang to compile any offloading application. Run ldd on it and see if there are any LLVM libraries listed in the dependencies.

[ye-luo]

If you want to make extra sure you can apply this patch and use the new clang to compile any offloading application. Run ldd on it and see if there are any LLVM libraries listed in the dependencies.

Tried it. It was clean.

[JonChesterfield]

Static linking llvm libs into libomptarget should remove any deployment concerns. The binary is then very similar to what we'd have if we copy/paste/reimplement instead of reuse.

[jhuber6]

In D129875#3661072, @ye-luo wrote:

If you want to make extra sure you can apply this patch and use the new clang to compile any offloading application. Run ldd on it and see if there are any LLVM libraries listed in the dependencies.

Tried it. It was clean.

Thanks, I will try to land this tomorrow and see what falls over.

[JonChesterfield]

Immediately took down the amdgpu buildbot with error while loading shared libraries: libomptarget.so.15, cannot open shared object file: No such file or directory, reverted while trying to guess why

@ronlieb thoughts?

[jhuber6]

Going to try to land this again, I believe the problems caused on the AMDGPU buildbot are primarily because the LLVM library installs things to ./lib in the build directory while the old library lived at ./openmp/libomptarget/ or similar.

[jhuber6]

Adding another installation directory. This will install the library at the previous location as well as the standard LLVM one. This isn't the ideal solution, but it's much less noisy until we totally migrate all the plugins and libraries. Once we do that we can just update the LIBOMPTARGET_LIBRARY_DIR variable to be LLVM_LIBRARY_DIR.

[jhuber6]

Opening again, hopefully third time's the charm.

[jhuber6]

Updating with new extra lit variables for the LLVM library rpath.

[kaz7]

Hi, I am having configuration error after this patch like it cannot find zlib and terminfo to compile libomptarget. Why those are required for libomptarget is that those libraries are listed in install/lib/cmake/llvm/LLVMConfig.cmake like below if I compile llvm with default setting. it's clear that libomptarget doesn't require them, but this patch change it to require similar libraries as llvm itself. I would like to disable them for only libomptarget compiling. Do you know how to do that? Please let me know any way to solve this situation. Thank you!

set(LLVM_ENABLE_TERMINFO TRUE)
if(LLVM_ENABLE_TERMINFO)
  find_package(Terminfo)
endif()

set(LLVM_ENABLE_ZLIB 1)
if(LLVM_ENABLE_ZLIB)
  set(ZLIB_ROOT )
  find_package(ZLIB)
endif()

[jhuber6]

In D129875#3704144, @kaz7 wrote:

Hi, I am having configuration error after this patch like it cannot find zlib and terminfo to compile libomptarget. Why those are required for libomptarget is that those libraries are listed in install/lib/cmake/llvm/LLVMConfig.cmake like below if I compile llvm with default setting. it's clear that libomptarget doesn't require them, but this patch change it to require similar libraries as llvm itself. I would like to disable them for only libomptarget compiling. Do you know how to do that? Please let me know any way to solve this situation. Thank you!

set(LLVM_ENABLE_TERMINFO TRUE)
if(LLVM_ENABLE_TERMINFO)
  find_package(Terminfo)
endif()

set(LLVM_ENABLE_ZLIB 1)
if(LLVM_ENABLE_ZLIB)
  set(ZLIB_ROOT )
  find_package(ZLIB)
endif()

Libomptarget now depends on the LLVM libraries , so whatever dependencies LLVM has libomptarget needs to have as well. Is it possible for you to build with -DLLVM_ENABLE_TERMINFO=OFF -DLLVM_ENABLE_ZLIB=OFF? I'd assume that would stop LLVM from looking for it and prevent libomptarget from picking it up either.

[kaz7]

In D129875#3704145, @jhuber6 wrote:

Libomptarget now depends on the LLVM libraries , so whatever dependencies LLVM has libomptarget needs to have as well. Is it possible for you to build with -DLLVM_ENABLE_TERMINFO=OFF -DLLVM_ENABLE_ZLIB=OFF? I'd assume that would stop LLVM from looking for it and prevent libomptarget from picking it up either.

Yes. Adding those options to not only libomptarget but also whole llvm solves this problem. Thank you.

But, do you really want to make libomptarget depends on LLVM libraries including terminfo and zlib although libomptarget is running on accelarators? Isn't it possible to separate LLVM libraries for hosts and for targets somehow? Just a thought.

[jhuber6]

In D129875#3704714, @kaz7 wrote:

In D129875#3704145, @jhuber6 wrote:

Libomptarget now depends on the LLVM libraries , so whatever dependencies LLVM has libomptarget needs to have as well. Is it possible for you to build with -DLLVM_ENABLE_TERMINFO=OFF -DLLVM_ENABLE_ZLIB=OFF? I'd assume that would stop LLVM from looking for it and prevent libomptarget from picking it up either.

Yes. Adding those options to not only libomptarget but also whole llvm solves this problem. Thank you.

But, do you really want to make libomptarget depends on LLVM libraries including terminfo and zlib although libomptarget is running on accelarators? Isn't it possible to separate LLVM libraries for hosts and for targets somehow? Just a thought.

LLVM builds and links static libraries by default, so unless changed by the user the LLVM libraries shouldn't be required by user applications using libomptarget. Is the problem here that LLVM is pulling in dependencies for libz and terminfo that aren't statically removed in the default build configuration? If that's the case it would be a good idea to try to remove those unless we explicitly need them.

[JonChesterfield]

libomptarget is running on accelarators

Libomptarget is host code. There's another library called DeviceRTL which runs on the accelerator and that won't be linking in llvm libs or zlib.

We could audit which llvm libs are transitively depended on and see whether they use zlib or terminfo and try to avoid them if necessary, but that's really better solved at the llvm libs level. Only declare the dependency on libs that use them. I'd guess that hasn't been worth doing so far so using any llvm lib is taken as a sign that you may as well link those two as well, unless you opted out using the top level defines. Doesn't look especially worth the cmake complexity and maintenance cost to me.

[kaz7]

In D129875#3704796, @JonChesterfield wrote:

Libomptarget is host code. There's another library called DeviceRTL which runs on the accelerator and that won't be linking in llvm libs or zlib.

Thank you for the correction! Then, I'm not sure why cmake for libomptarget fails to find terminfo and zlib although those are used by llvm itself.

We could audit which llvm libs are transitively depended on and see whether they use zlib or terminfo and try to avoid them if necessary, but that's really better solved at the llvm libs level. Only declare the dependency on libs that use them. I'd guess that hasn't been worth doing so far so using any llvm lib is taken as a sign that you may as well link those two as well, unless you opted out using the top level defines. Doesn't look especially worth the cmake complexity and maintenance cost to me.

That's true. It's much easier and I agree with you. Ok. Thanks!

openmp/libomptarget/CMakeLists.txt

 set(LIBOMPTARGET_SRC_DIR ${CMAKE_CURRENT_SOURCE_DIR}/src)
 add_subdirectory(${LIBOMPTARGET_SRC_DIR})
 
 # Definitions for testing, for reuse when testing libomptarget-nvptx.
 set(LIBOMPTARGET_OPENMP_HEADER_FOLDER "${LIBOMP_INCLUDE_DIR}" CACHE STRING
   "Path to folder containing omp.h")
 set(LIBOMPTARGET_OPENMP_HOST_RTL_FOLDER "${LIBOMP_LIBRARY_DIR}" CACHE STRING
   "Path to folder containing libomp.so, and libLLVMSupport.so with profiling enabled")
-
+set(LIBOMPTARGET_LLVM_LIBRARY_DIR "${LLVM_LIBRARY_DIR}" CACHE STRING
+  "Path to folder containing llvm library libomptarget.so")
 
 # Build offloading plugins and device RTLs if they are available.
 add_subdirectory(plugins)
 add_subdirectory(DeviceRTL)
 add_subdirectory(tools)
 
 # Add tests.
 add_subdirectory(test)

openmp/libomptarget/src/CMakeLists.txt

 ##===----------------------------------------------------------------------===##
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 #
 ##===----------------------------------------------------------------------===##
 #
 # Build offloading library libomptarget.so.
 #
 ##===----------------------------------------------------------------------===##
 
 libomptarget_say("Building offloading runtime library libomptarget.")
 
-set(LIBOMPTARGET_SRC_FILES
-  ${CMAKE_CURRENT_SOURCE_DIR}/api.cpp
-  ${CMAKE_CURRENT_SOURCE_DIR}/device.cpp
-  ${CMAKE_CURRENT_SOURCE_DIR}/interface.cpp
-  ${CMAKE_CURRENT_SOURCE_DIR}/interop.cpp
-  ${CMAKE_CURRENT_SOURCE_DIR}/omptarget.cpp
-  ${CMAKE_CURRENT_SOURCE_DIR}/rtl.cpp
-  ${CMAKE_CURRENT_SOURCE_DIR}/LegacyAPI.cpp
-)
+add_llvm_library(omptarget
+  SHARED
+
+  api.cpp
+  device.cpp
+  interface.cpp
+  interop.cpp
+  omptarget.cpp
+  rtl.cpp
+  LegacyAPI.cpp
 
-set(LIBOMPTARGET_SRC_FILES ${LIBOMPTARGET_SRC_FILES} PARENT_SCOPE)
+  ADDITIONAL_HEADER_DIRS
+  ${LIBOMPTARGET_INCLUDE_DIR}
 
-# Build libomptarget library with libdl dependency.
-add_library(omptarget SHARED ${LIBOMPTARGET_SRC_FILES})
-set_target_properties(omptarget PROPERTIES INSTALL_RPATH "$ORIGIN" BUILD_RPATH "$ORIGIN")
-if (OPENMP_ENABLE_LIBOMPTARGET_PROFILING)
-  # Add LLVMSupport dependency if profiling is enabled.
-  # Linking with LLVM component libraries also requires
-  # aligning the compile flags.
-  llvm_update_compile_flags(omptarget)
-  target_compile_definitions(omptarget PUBLIC OMPTARGET_PROFILE_ENABLED)
-  target_link_libraries(omptarget PRIVATE LLVMSupport)
-endif()
-target_include_directories(omptarget PRIVATE
-  ${LIBOMPTARGET_INCLUDE_DIR})
-target_link_libraries(omptarget PRIVATE
+  LINK_COMPONENTS
+  Support
+  Object
+
+  LINK_LIBS
   ${CMAKE_DL_LIBS}
-  "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/exports")
+  "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/exports"
+  NO_INSTALL_RPATH
+)
+target_include_directories(omptarget PRIVATE ${LIBOMPTARGET_INCLUDE_DIR})
 
-# Install libomptarget under the lib destination folder.
-install(TARGETS omptarget LIBRARY COMPONENT omptarget
-  DESTINATION "${OPENMP_INSTALL_LIBDIR}")
+# libomptarget.so needs to be aware of where the plugins live as they
+# are now separated in the build directory.
+set_target_properties(omptarget PROPERTIES INSTALL_RPATH "$ORIGIN" BUILD_RPATH "$ORIGIN:${CMAKE_CURRENT_BINARY_DIR}/..")
+install(TARGETS omptarget LIBRARY COMPONENT omptarget DESTINATION "${OPENMP_INSTALL_LIBDIR}")

openmp/libomptarget/test/lit.cfg

elif config.operating_system == 'Darwin':
     append_dynamic_library_path('DYLD_LIBRARY_PATH', config.library_dir, ":")
     append_dynamic_library_path('DYLD_LIBRARY_PATH', \
         config.omp_host_rtl_directory, ";")
     config.test_flags += " -Wl,-rpath," + config.library_dir
     config.test_flags += " -Wl,-rpath," + config.omp_host_rtl_directory
 else: # Unices
     config.test_flags += " -Wl,-rpath," + config.library_dir
     config.test_flags += " -Wl,-rpath," + config.omp_host_rtl_directory
+    config.test_flags += " -Wl,-rpath," + config.llvm_lib_directory
     if config.cuda_libdir:
         config.test_flags += " -Wl,-rpath," + config.cuda_libdir
     if config.libomptarget_current_target.startswith('amdgcn'):
         config.test_flags += " --libomptarget-amdgcn-bc-path=" + config.library_dir
     if config.libomptarget_current_target.startswith('nvptx'):
         config.test_flags += " --libomptarget-nvptx-bc-path=" + config.library_dir
     if config.libomptarget_current_target.endswith('-oldDriver'):
         config.test_flags += " -fno-openmp-new-driver"

openmp/libomptarget/test/lit.site.cfg.in

 @AUTO_GEN_COMMENT@
 
 config.test_c_compiler = "@OPENMP_TEST_C_COMPILER@"
 config.test_cxx_compiler = "@OPENMP_TEST_CXX_COMPILER@"
 config.test_compiler_features = @OPENMP_TEST_COMPILER_FEATURES@
 config.test_openmp_flags = "@OPENMP_TEST_OPENMP_FLAGS@"
 config.test_extra_flags = "@OPENMP_TEST_FLAGS@"
 config.cuda_path = "@CUDA_TOOLKIT_ROOT_DIR@"
 config.cuda_libdir = "@CUDA_LIBDIR@"
 config.cuda_test_arch = "@LIBOMPTARGET_DEP_CUDA_ARCH@"
 config.libomptarget_obj_root = "@CMAKE_CURRENT_BINARY_DIR@/@CURRENT_TARGET@"
 config.library_dir = "@LIBOMPTARGET_LIBRARY_DIR@"
 config.omp_header_directory = "@LIBOMPTARGET_OPENMP_HEADER_FOLDER@"
 config.omp_host_rtl_directory = "@LIBOMPTARGET_OPENMP_HOST_RTL_FOLDER@"
+config.llvm_lib_directory = "@LIBOMPTARGET_LLVM_LIBRARY_DIR@"
 config.operating_system = "@CMAKE_SYSTEM_NAME@"
 config.libomptarget_all_targets = "@LIBOMPTARGET_ALL_TARGETS@".split()
 config.libomptarget_current_target = "@CURRENT_TARGET@"
 config.libomptarget_filecheck = "@OPENMP_FILECHECK_EXECUTABLE@"
 config.libomptarget_not = "@OPENMP_NOT_EXECUTABLE@"
 config.libomptarget_debug = @LIBOMPTARGET_DEBUG@
 
 # Let the main config do the real work.
 lit_config.load_config(config, "@CMAKE_CURRENT_SOURCE_DIR@/lit.cfg")