Skip to content

[19.x] Backport standalone build fixes for offload #118643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Dec 17, 2024

Conversation

mgorny
Copy link
Member

@mgorny mgorny commented Dec 4, 2024

Backport the changes from #104647 and part of the changes from #118173. Note that I've cherry-picked the individual changes here since merging #118173 wholesale makes automatic merge adds some stuff that weren't present in 19.x. With these changes, I can build and run x86-64 tests of offload in a standalone build, i.e. 19.x becomes in line with main.

@mgorny mgorny added this to the LLVM 19.X Release milestone Dec 4, 2024
@llvmbot llvmbot added the offload label Dec 4, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 4, 2024

@llvm/pr-subscribers-offload

Author: Michał Górny (mgorny)

Changes

Backport the changes from #104647 and part of the changes from #118173. Note that I've cherry-picked the individual changes here since merging #118173 wholesale makes automatic merge adds some stuff that weren't present in 19.x. With these changes, I can build and run x86-64 tests of offload in a standalone build, i.e. 19.x becomes in line with main.


Full diff: https://github.com/llvm/llvm-project/pull/118643.diff

4 Files Affected:

  • (modified) offload/CMakeLists.txt (+21)
  • (modified) offload/cmake/OpenMPTesting.cmake (+1-1)
  • (modified) offload/plugins-nextgen/common/CMakeLists.txt (+9-7)
  • (modified) offload/test/CMakeLists.txt (+8-3)
diff --git a/offload/CMakeLists.txt b/offload/CMakeLists.txt
index 4cd97a6a5ff63d..959d6260bc7490 100644
--- a/offload/CMakeLists.txt
+++ b/offload/CMakeLists.txt
@@ -127,6 +127,7 @@ include(LibomptargetGetDependencies)
 # Set up testing infrastructure.
 include(OpenMPTesting)
 
+include(CheckCXXCompilerFlag)
 check_cxx_compiler_flag(-Werror=global-constructors OFFLOAD_HAVE_WERROR_CTOR)
 
 # LLVM source tree is required at build time for libomptarget
@@ -282,6 +283,26 @@ if(OPENMP_STANDALONE_BUILD)
       ${LLVM_LIBRARY_DIRS}
     REQUIRED
   )
+
+  find_path (
+    LIBOMP_INCLUDE_DIR
+    NAMES
+      omp.h
+    HINTS
+    ${COMPILER_RESOURCE_DIR}/include
+    ${CMAKE_INSTALL_PREFIX}/include
+  )
+
+  get_filename_component(LIBOMP_LIBRARY_DIR ${LIBOMP_STANDALONE} DIRECTORY)
+
+  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(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")
 endif()
 
 macro(pythonize_bool var)
diff --git a/offload/cmake/OpenMPTesting.cmake b/offload/cmake/OpenMPTesting.cmake
index 11eafeb764260f..3e04a3423c4d64 100644
--- a/offload/cmake/OpenMPTesting.cmake
+++ b/offload/cmake/OpenMPTesting.cmake
@@ -124,7 +124,7 @@ if (${OPENMP_STANDALONE_BUILD})
   # project is built which is too late for detecting the compiler...
   file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/DetectTestCompiler)
   execute_process(
-    COMMAND ${CMAKE_COMMAND} -G${CMAKE_GENERATOR} ${CMAKE_CURRENT_LIST_DIR}/DetectTestCompiler
+    COMMAND ${CMAKE_COMMAND} -G${CMAKE_GENERATOR} ${CMAKE_CURRENT_SOURCE_DIR}/../openmp/cmake/DetectTestCompiler
       -DCMAKE_C_COMPILER=${OPENMP_TEST_C_COMPILER}
       -DCMAKE_CXX_COMPILER=${OPENMP_TEST_CXX_COMPILER}
     WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/DetectTestCompiler
diff --git a/offload/plugins-nextgen/common/CMakeLists.txt b/offload/plugins-nextgen/common/CMakeLists.txt
index 284f98875170cd..aea20c6ec31435 100644
--- a/offload/plugins-nextgen/common/CMakeLists.txt
+++ b/offload/plugins-nextgen/common/CMakeLists.txt
@@ -11,13 +11,15 @@ add_dependencies(PluginCommon intrinsics_gen)
 
 # Only enable JIT for those targets that LLVM can support.
 set(supported_jit_targets AMDGPU NVPTX)
-foreach(target IN LISTS supported_jit_targets)
-  if("${target}" IN_LIST LLVM_TARGETS_TO_BUILD)
-	  target_compile_definitions(PluginCommon PRIVATE "LIBOMPTARGET_JIT_${target}")
-    llvm_map_components_to_libnames(llvm_libs ${target})
-    target_link_libraries(PluginCommon PRIVATE ${llvm_libs})
-  endif()
-endforeach()
+if (NOT LLVM_LINK_LLVM_DYLIB)
+  foreach(target IN LISTS supported_jit_targets)
+    if("${target}" IN_LIST LLVM_TARGETS_TO_BUILD)
+      target_compile_definitions(PluginCommon PRIVATE "LIBOMPTARGET_JIT_${target}")
+      llvm_map_components_to_libnames(llvm_libs ${target})
+      target_link_libraries(PluginCommon PRIVATE ${llvm_libs})
+    endif()
+  endforeach()
+endif()
 
 # Include the RPC server from the `libc` project if availible.
 if(TARGET llvmlibc_rpc_server AND ${LIBOMPTARGET_GPU_LIBC_SUPPORT})
diff --git a/offload/test/CMakeLists.txt b/offload/test/CMakeLists.txt
index 3ac5d7907e2cc2..c90ed26389faf6 100644
--- a/offload/test/CMakeLists.txt
+++ b/offload/test/CMakeLists.txt
@@ -22,6 +22,11 @@ if(CUDAToolkit_FOUND)
   get_filename_component(CUDA_LIBDIR "${CUDA_cudart_static_LIBRARY}" DIRECTORY)
 endif()
 
+set(OMP_DEPEND)
+if(TARGET omp)
+  set(OMP_DEPEND omp)
+endif()
+
 string(REGEX MATCHALL "([^\ ]+\ |[^\ ]+$)" SYSTEM_TARGETS "${LIBOMPTARGET_SYSTEM_TARGETS}")
 foreach(CURRENT_TARGET IN LISTS SYSTEM_TARGETS)
   string(STRIP "${CURRENT_TARGET}" CURRENT_TARGET)
@@ -29,7 +34,7 @@ foreach(CURRENT_TARGET IN LISTS SYSTEM_TARGETS)
   add_offload_testsuite(check-libomptarget-${CURRENT_TARGET}
     "Running libomptarget tests"
     ${CMAKE_CURRENT_BINARY_DIR}/${CURRENT_TARGET}
-    DEPENDS omptarget omp ${LIBOMPTARGET_TESTED_PLUGINS}
+    DEPENDS omptarget ${OMP_DEPEND} ${LIBOMPTARGET_TESTED_PLUGINS}
     ARGS ${LIBOMPTARGET_LIT_ARG_LIST})
   list(APPEND LIBOMPTARGET_LIT_TESTSUITES ${CMAKE_CURRENT_BINARY_DIR}/${CURRENT_TARGET})
 
@@ -43,12 +48,12 @@ add_offload_testsuite(check-libomptarget
   "Running libomptarget tests"
   ${LIBOMPTARGET_LIT_TESTSUITES}
   EXCLUDE_FROM_CHECK_ALL
-  DEPENDS omptarget omp ${LIBOMPTARGET_TESTED_PLUGINS}
+  DEPENDS omptarget ${OMP_DEPEND} ${LIBOMPTARGET_TESTED_PLUGINS}
   ARGS ${LIBOMPTARGET_LIT_ARG_LIST})
 
 add_offload_testsuite(check-offload
   "Running libomptarget tests"
   ${LIBOMPTARGET_LIT_TESTSUITES}
   EXCLUDE_FROM_CHECK_ALL
-  DEPENDS omptarget omp ${LIBOMPTARGET_TESTED_PLUGINS}
+  DEPENDS omptarget ${OMP_DEPEND} ${LIBOMPTARGET_TESTED_PLUGINS}
   ARGS ${LIBOMPTARGET_LIT_ARG_LIST})

estewart08 and others added 6 commits December 17, 2024 10:08
Error: CommandLine Error: Option 'attributor-manifest-internal'
registered more than once

During the standalone debug build of offload the above error is seen at
app runtime when using a prebuilt llvm with LLVM_LINK_LLVM_DYLIB=ON.
This is caused by linking both libLLVM.so and various archives that are
found via llvm_map_components_to_libnames for jit support.
Include CheckCXXCompilerFlag before it is used in the top-level CMake
file.  This fixes standalone builds, but it is also cleaner than
assuming that some previous CMake file will include it for us.
Add the `omp` target dependency to tests only when the respective target
is present, i.e. when not building standalone against system libomp.
Copy the `OPENMP_TEST_FLAGS` and `OPENMP_TEST_OPENMP_FLAGS` from openmp
for standalone builds, as required by the lit configs.
Define `LIBOMPTARGET_OPENMP_HEADER_FOLDER`
and `LIBOMPTARGET_OPENMP_HOST_RTL_FOLDER` needed for testing
in standalone builds.
Fix the DetectTestCompiler project use to reference the openmp source
tree, since the respective files were not copied to offload, and there
is no point in duplicating them.

Fixes llvm#90333
@tru tru merged commit 2384a06 into llvm:release/19.x Dec 17, 2024
4 of 6 checks passed
Copy link

@mgorny (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

@mgorny mgorny deleted the standalone-19.x branch December 17, 2024 10:39
@mgorny
Copy link
Member Author

mgorny commented Dec 17, 2024

Thanks! Since it's a nice setup, and pretty much was broken before, I don't think there's a point in a release note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

5 participants