[Openmp-commits] [PATCH] D14254: [OpenMP] Initial implementation of OpenMP offloading library - libomptarget device RTLs.

[Hal Finkel via Phabricator via Openmp-commits]

hfinkel added inline comments.


================
Comment at: libomptarget/deviceRTLs/nvptx/CMakeLists.txt:124-139
+    if (NOT LIBOMPTARGET_NVPTX_BC_LINKER STREQUAL "")
+      set(LIBOMPTARGET_NVPTX_SELECTED_BC_LINKER ${LIBOMPTARGET_NVPTX_BC_LINKER})
+    elseif(NOT ${LIBOMPTARGET_STANDALONE_BUILD})
+        if(MSVC)
+          set(LIBOMPTARGET_NVPTX_SELECTED_BC_LINKER ${LLVM_TOOLS_BINARY_DIR}/llvm-link.exe)
+        else()
+          set(LIBOMPTARGET_NVPTX_SELECTED_BC_LINKER ${LLVM_TOOLS_BINARY_DIR}/llvm-link)
----------------
grokos wrote:
> Hahnfeld wrote:
> > grokos wrote:
> > > Hahnfeld wrote:
> > > > 1. Do not depend on in-tree components.
> > > > 2. I think you should be able to do the "link" step with Clang as well, can you please test this?
> > >   # Changed the linker-setting logic to the same as the compiler-setting logic above.
> > >   # Clang can be used to link but with different command line options. For consistency I am sticking to `llvm-link`. My rationale is that if we are building libomptarget with clang then `llvm-link` must also be around, so if `CMAKE_C_COMPILER_ID` equals "Clang", then use `llvm-link`.
> > > 
> > I disagree: `CMAKE_C_COMPILER` doesn't need to be in `PATH` so we might not be able to find `llvm-link` or end up with a different installation. And using `clang` for linking would be consistent with the usual behavior (build system invokes compiler with object files and the compiler driver invokes the linker). Conclusion: It is better and possible to use `clang` for linking and we should do it.
> After looking into this, from what I could find clang cannot link bitcode files into a single bitcode file - this can only be done with `llvm-link`. What clang can do is link multiple `.bc` files into a target-specific object file (which is not what we want here) and for this it needs `libLTO` and the `LLVM gold` plugin (which are not necessarily installed on every system and asking the user to provide paths makes things more complex). If anyone knows of a simple way to make clang link multiple `.bc` files into a single bitcode library, please let me know. Otherwise, I'm afraid we'll have to stick to `llvm-link`.
I don't believe that Clang itself has any configuration in which it will do what llvm-link does. I'd be in favor of such a mode, and maybe this is a good motivating use case, but others might disagree (i.e., if you're working with bitcode, then we might assume that you have the LLVM utilities around).

We should probably search for llvm-link first in the directory containing clang (as it, indeed, might not be in the user's path). We would need to document, moreover, that LIBOMPTARGET_NVPTX_SELECTED_BC_LINKER need to be set to llvm-link from the right installation if there's not one in Clang's bin directory and the one in the path is not right.


Repository:
  rL LLVM

https://reviews.llvm.org/D14254