[HIP] fix paths for executables not in clang bin directory

[DieGoldeneEnte]

Summary

This patch now only adds the executable dirs to the program path, the code to search them is now in D72903.

The previous code constructed the executable paths for llvm-link, opt, lld, llc and clang-offload-bundler from the path to the compiler. This change uses cmake to find the directory containing llvm and lld and adds them to the ProgramPath.

The test completed without error (except one which failed even without my patch) and I was able to confirm successful compilation using the hip toolchain by compiling a simple program with clang, lld, and llvm executables in different directories.

[yaxunl]

What's the use case of this change?

Normally clang needs to call opt/llc/lld from the same directory of clang. Why do we need to find them in other directories?

Where is TOOLS_BINARY_DIR defined?

Also we cannot let the build fail because of lld not found, since users may be building clang for other language and they do not need lld.

[tra]

LGTM. Added @beanz for the review of CMake file changes.

[DieGoldeneEnte]

In D72806#1824560, @yaxunl wrote:

What's the use case of this change?

Normally clang needs to call opt/llc/lld from the same directory of clang. Why do we need to find them in other directories?

My motivation is the nix-package manager, which has llvm, lld and clang in different packages (which results in different directories).

Where is TOOLS_BINARY_DIR defined?

If clang is compiled standalone with llvm as external library the TOOLS_BINARY_DIR is pulled from the LLVMConfig.cmake. If clang is compiled together with llvm the variable is set in the llvm/CMakeLists.txt .
Upon closer inspection I noticed I can/should use LLVM_TOOLS_BINARY_DIR (which is more readable). In the end it points to the llvm/bin directory.

Also we cannot let the build fail because of lld not found, since users may be building clang for other language and they do not need lld.

Removed the message, so build doesn't fail. Since I check for the existence of the define in the code only the cmake rules needed to be changed.

[tra]

In D72806#1825333, @DieGoldeneEnte wrote:

In D72806#1824560, @yaxunl wrote:

What's the use case of this change?

Normally clang needs to call opt/llc/lld from the same directory of clang. Why do we need to find them in other directories?

My motivation is the nix-package manager, which has llvm, lld and clang in different packages (which results in different directories).

We've had similar issues with use of clang for CUDA on Debian (I think), where CUDA was scattered all over the place.
The way to do it was to create a 'shim' directory structure which would put all relevant tools in the right places.
Something similar could be done in your case -- make a package which would depend on individual tool packages and which would symlink all of them into one dir and point your compiler there.

[DieGoldeneEnte]

The build doesn't fail anymore if lld is not present, also one can set LLD_BINARY_DIR manually. I also exchanged TOOLS_BINARY_DIR with LLVM_TOOLS_BINARY_DIR, since it is better readable and if compiled with llvm this is necessary to populate the variable (although the path is already present in that case, because clang and all the other utils should be in the same directory.

Tests still work as before.

[DieGoldeneEnte]

In D72806#1825362, @tra wrote:

In D72806#1825333, @DieGoldeneEnte wrote:

In D72806#1824560, @yaxunl wrote:

What's the use case of this change?

Normally clang needs to call opt/llc/lld from the same directory of clang. Why do we need to find them in other directories?

My motivation is the nix-package manager, which has llvm, lld and clang in different packages (which results in different directories).

We've had similar issues with use of clang for CUDA on Debian (I think), where CUDA was scattered all over the place.
The way to do it was to create a 'shim' directory structure which would put all relevant tools in the right places.
Something similar could be done in your case -- make a package which would depend on individual tool packages and which would symlink all of them into one dir and point your compiler there.

That's what I will do if we don't fix this here. But I think it would be cleaner if one can set the directories.

Even in case we don't add the extra directories for llvm and lld, it would be a good idea to use the getProgramPath function instead of building it manually (so only the changes in HIP.cpp except lines 273-282). This was planned anyways according to the comments in HIP.cpp line 270-271:

Lookup binaries into the driver directory, this is used to
discover the clang-offload-bundler executable.

[tra]

In D72806#1825400, @DieGoldeneEnte wrote:

Even in case we don't add the extra directories for llvm and lld, it would be a good idea to use the getProgramPath function instead of building it manually (so only the changes in HIP.cpp except lines 273-282). This was planned anyways according to the comments in HIP.cpp line 270-271:

Lookup binaries into the driver directory, this is used to
discover the clang-offload-bundler executable.

Agreed. GetProgramPath() change makes sense on its own merits.

In the future it may help to keep intependent changes in separate patches. It makes it easier to review and land them that way. I.e. the patch with GetProgramPath() would be stamped w/o problems as it's an obvious clean up local to HIP only. Changing build for everyone will need more scrutiny and would be easier to deal with if it's not commingled with other changes.

It's not too late to split this patch in two. :-)

[DieGoldeneEnte]

This patch now only adds the executable dirs to the program path, the code to search them is now in D72903.

[tra]

Tank you for splitting the patch. For CMake parts @beanz would be the person to make the call.

[DieGoldeneEnte]

Adding the paths for llvm/lld is not needed, because GetProgramPath is actually also searching in $PATH. This means D72903 already is enough to fix my problem.

clang/CMakeLists.txt

if (LLVM_ADD_NATIVE_VISUALIZERS_TO_SOLUTION)
   add_subdirectory(utils/ClangVisualizers)
 endif()
 add_subdirectory(utils/hmaptool)
 
 if(CLANG_BUILT_STANDALONE)
   llvm_distribution_add_targets()
 endif()
 
+# add LLVM_TOOLS_BINARY_DIR to Defines to make llc, llvm-link and opt available
+# for HIP toolchain
+if(DEFINED LLVM_TOOLS_BINARY_DIR)
+  add_definitions( -DLLVM_TOOLS_BINARY_DIR="${LLVM_TOOLS_BINARY_DIR} " )
+  message(STATUS "found llvm executable dir: ${LLVM_TOOLS_BINARY_DIR}")
+endif()
+
+# add LLD_BINARY_DIR to Defines to make lld available for HIP toolchain
+find_program(LLD_BINARY NAMES lld)
+if(NOT LLD_BINARY MATCHES "-NOTFOUND" AND NOT DEFINED LLD_BINARY_DIR)
+  message(STATUS "found lld executable: ${LLD_BINARY}")
+  get_filename_component(LLD_BINARY_DIR ${LLD_BINARY} DIRECTORY)
+endif()
+if(DEFINED LLD_BINARY)
+  add_definitions( -DLLD_BINARY_DIR="${LLD_BINARY_DIR} " )
+endif()
+
+
 configure_file(
   ${CLANG_SOURCE_DIR}/include/clang/Config/config.h.cmake
   ${CLANG_BINARY_DIR}/include/clang/Config/config.h)

clang/lib/Driver/ToolChains/HIP.cpp

 }
 
 HIPToolChain::HIPToolChain(const Driver &D, const llvm::Triple &Triple,
                              const ToolChain &HostTC, const ArgList &Args)
     : ToolChain(D, Triple, Args), HostTC(HostTC) {
   // Lookup binaries into the driver directory, this is used to
   // discover the clang-offload-bundler executable.
   getProgramPaths().push_back(getDriver().Dir);
+
+// add llvm binaries in case they are not in the driver directory
+#if defined(LLVM_TOOLS_BINARY_DIR)
+  getProgramPaths().push_back(LLVM_TOOLS_BINARY_DIR);
+#endif
+
+// add lld binary in case they are not in the driver directory
+#if defined(LLD_BINARY_DIR)
+  getProgramPaths().push_back(LLD_BINARY_DIR);
+#endif
 }
 
 void HIPToolChain::addClangTargetOptions(
     const llvm::opt::ArgList &DriverArgs,
     llvm::opt::ArgStringList &CC1Args,
     Action::OffloadKind DeviceOffloadingKind) const {
   HostTC.addClangTargetOptions(DriverArgs, CC1Args, DeviceOffloadingKind);