[libomptarget][amdcgn] Add build dependency for opt

[protze.joachim]

Summary

This change fixes the build error I observe when building LLVM from scratch.

[tianshilei1992]

The link of bc lib requires opt?

[protze.joachim]

The command uses | ${OPT_TOOL}. If I build OpenMP as llvm runtime, I regularly get the error, that ${OPT_TOOL} cannot be found, because opt was not built at that time.
Rerunning the parallel build finally succeeds, but is annoying

[ronlieb]

thank you for fixing this.
i see the OPT failure about 40% of the time when i am building amdgpu libomptarget.

[tianshilei1992]

LGTM. Thanks.

[protze.joachim]

If this fixes the build issues for everyone, I'll ping Tom for backporting to the release/13 branch.

[JonChesterfield]

Ah, so that's how to declare the dependency. We almost certainly want to depend on llvm-link too. Should that be spelled ${OPT_TOOL} and ${LINK_TOOL}?

[tianshilei1992]

In D107156#2916950, @JonChesterfield wrote:

Ah, so that's how to declare the dependency. We almost certainly want to depend on llvm-link too. Should that be spelled ${OPT_TOOL} and ${LINK_TOOL}?

opt can be a target or the path to a binary, but ${OPT_TOOL} and ${LINK_TOOL} are only path.

[JonChesterfield]

Therefore it must be spelled opt here? I.e. we should add llvm-link, not the braced form?

[tianshilei1992]

In D107156#2917204, @JonChesterfield wrote:

Therefore it must be spelled opt here? I.e. we should add llvm-link, not the braced form?

Well, technically, we need to check if we are in runtime build. If yes, set up the dependence; otherwise, check if the path is valid.

[protze.joachim]

I was wondering, why cmake does not complain about opt as an unknown dependency in any of the different build configurations (project/runtime/standalone). Could it be the case, that the cmake target comes from find_package(LLVM)?

[tianshilei1992]

In D107156#2918051, @protze.joachim wrote:

I was wondering, why cmake does not complain about opt as an unknown dependency in any of the different build configurations (project/runtime/standalone). Could it be the case, that the cmake target comes from find_package(LLVM)?

Actually it is exactly how targets in runtime build are from.

[raj.khem]

This breaks a case where opt is used from installed locations especially in standalone cross-build environment we pass -DOPT_TOOL=${STAGING_BINDIR_NATIVE}/opt and it worked ok but now its expecting opt to be built along with openmp build and fails like below

NOTE: cmake --build TOPDIR/build/tmp/work/cortexa57-yoe-linux-musl/openmp/13.0.0-r0/build --target all -- ninja: error: 'libomptarget/deviceRTLs/amdgcn/opt', needed by 'libomptarget/deviceRTLs/amdgcn/libomptarget-amdgcn-gfx701.bc', missing and no known rule to make it

how can be solve this now ?

[JonChesterfield]

The current trunk cmake guards whether to build this with:

if (LLVM_DIR)
  # Builds that use pre-installed LLVM have LLVM_DIR set.                                                                                                                                                          
  find_program(CLANG_TOOL clang PATHS ${LLVM_TOOLS_BINARY_DIR} NO_DEFAULT_PATH)
  find_program(LINK_TOOL llvm-link PATHS ${LLVM_TOOLS_BINARY_DIR}
    NO_DEFAULT_PATH)
  find_program(OPT_TOOL opt PATHS ${LLVM_TOOLS_BINARY_DIR} NO_DEFAULT_PATH)
  libomptarget_say("Building AMDGCN device RTL. Using clang: ${CLANG_TOOL}")
elseif (LLVM_TOOL_CLANG_BUILD AND NOT CMAKE_CROSSCOMPILING AND NOT OPENMP_STANDALONE_BUILD)
  # LLVM in-tree builds may use CMake target names to discover the tools.                                                                                                                                          
  set(CLANG_TOOL $<TARGET_FILE:clang>)
  set(LINK_TOOL $<TARGET_FILE:llvm-link>)
  set(OPT_TOOL $<TARGET_FILE:opt>)
  libomptarget_say("Building AMDGCN device RTL. Using clang from in-tree build")
else()
  libomptarget_say("Not building AMDGCN device RTL. No appropriate clang found")
  return()
endif()

standalone cross-build

Which path are you hitting? Can grep for 'Building AMDGCN device RTL' to see which.

Guessing somewhat, we're specifying opt as a dependency whenever building this library, but perhaps the builds using pre-installed LLVM using LLVM_DIR must not specify it as a dependency, since it isn't supposed to be built as part of the current cmake invocation.

I'd still rather we only attempt to compile this as part of ENABLE_RUNTIMES. Pre-installed LLVM isn't especially likely to build an openmp offloading toolchain that works. If you can confirm that it's the if (LLVM_DIR) predicate that is passing, perhaps the direct fix is to replace find_program() with return()

[raj.khem]

In D107156#2970143, @JonChesterfield wrote:

The current trunk cmake guards whether to build this with:

if (LLVM_DIR)
  # Builds that use pre-installed LLVM have LLVM_DIR set.                                                                                                                                                          
  find_program(CLANG_TOOL clang PATHS ${LLVM_TOOLS_BINARY_DIR} NO_DEFAULT_PATH)
  find_program(LINK_TOOL llvm-link PATHS ${LLVM_TOOLS_BINARY_DIR}
    NO_DEFAULT_PATH)
  find_program(OPT_TOOL opt PATHS ${LLVM_TOOLS_BINARY_DIR} NO_DEFAULT_PATH)
  libomptarget_say("Building AMDGCN device RTL. Using clang: ${CLANG_TOOL}")
elseif (LLVM_TOOL_CLANG_BUILD AND NOT CMAKE_CROSSCOMPILING AND NOT OPENMP_STANDALONE_BUILD)
  # LLVM in-tree builds may use CMake target names to discover the tools.                                                                                                                                          
  set(CLANG_TOOL $<TARGET_FILE:clang>)
  set(LINK_TOOL $<TARGET_FILE:llvm-link>)
  set(OPT_TOOL $<TARGET_FILE:opt>)
  libomptarget_say("Building AMDGCN device RTL. Using clang from in-tree build")
else()
  libomptarget_say("Not building AMDGCN device RTL. No appropriate clang found")
  return()
endif()

standalone cross-build

Which path are you hitting? Can grep for 'Building AMDGCN device RTL' to see which.

I am on 13.x branch, This patch got backported to 13.x recently. I am not sure if trunk piece you are mentioning is also there in 13.x as well.

Guessing somewhat, we're specifying opt as a dependency whenever building this library, but perhaps the builds using pre-installed LLVM using LLVM_DIR must not specify it as a dependency, since it isn't supposed to be built as part of the current cmake invocation.

I'd still rather we only attempt to compile this as part of ENABLE_RUNTIMES. Pre-installed LLVM isn't especially likely to build an openmp offloading toolchain that works. If you can confirm that it's the if (LLVM_DIR) predicate that is passing, perhaps the direct fix is to replace find_program() with return()

LLVM_DIR is set for me

CMakeCache.txt:LLVM_DIR:PATH=/mnt/b/yoe/master/build/tmp/work/cortexa72-cortexa53-crypto-yoe-linux-musl/openmp/13.0.0-r0/recipe-sysroot/usr/lib/cmake/llvm

[protze.joachim]

I think, the proper solution is D108868: only add the dependency, if the tools are built at the same time (i.e., the corresponding build targets exist)

openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt

 foreach(mcpu ${mcpus})
   set(bc_files)
   add_cuda_bc_library(${cuda_sources})
 
   set(bc_libname lib${libname}-${mcpu}.bc)
   add_custom_command(
     OUTPUT ${bc_libname}
     COMMAND ${LINK_TOOL} ${bc_files} | ${OPT_TOOL} --always-inline -o ${OUTPUTDIR}/${bc_libname}
-    DEPENDS ${bc_files})
+    DEPENDS ${bc_files} opt)
 
   add_custom_target(lib${libname}-${mcpu} ALL DEPENDS ${bc_libname})
 
   install(FILES ${OUTPUTDIR}/${bc_libname}
      DESTINATION "${OPENMP_INSTALL_LIBDIR}"
   )
 endforeach()