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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld 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)
----------------
hfinkel wrote:
> 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.
@grokos Looks like you are right. I agree with @hfinkel, I would have also proposed searching in the directory of the `clang` binary. I think we might even omit searching in `PATH` and just defer to the user so we can avoid any trouble with incompatible tools. Thoughts?


Repository:
  rL LLVM

https://reviews.llvm.org/D14254