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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld requested changes to this revision.
Hahnfeld added a comment.
This revision now requires changes to proceed.

In https://reviews.llvm.org/D14254#960557, @gtbercea wrote:

> Good to go @Hahnfeld ?


No, I didn't have time to look at the latest changes due to travel. I've already added some commets to the build system that I still think is not optimal yet.



================
Comment at: libomptarget/deviceRTLs/nvptx/CMakeLists.txt:116-122
+    else()
+      find_program(LIBOMPTARGET_NVPTX_SELECTED_CUDA_COMPILER clang++)
+      if(NOT LIBOMPTARGET_NVPTX_SELECTED_CUDA_COMPILER)
+        libomptarget_say("Cannot find a CUDA compiler capable of emitting LLVM bitcode.")
+        libomptarget_say("Please configure with flag -DLIBOMPTARGET_NVPTX_CUDA_COMPILER")
+      endif()
+    endif()
----------------
grokos wrote:
> Hahnfeld wrote:
> > (Probably this case will go then too...)
> I left it there. The logic now is as follows:
> 
>   # If the user has specified a compiler via `-DLIBOMPTARGET_NVPTX_CUDA_COMPILER` then we'll use that.
>   # If not, then we check whether `CMAKE_C_COMPILER` is clang; if yes, then we'll use that.
>   # Otherwise, we try to search for a clang compiler. If we don't find any, then we don't build bclib.
> 
> 
I still don't like searching a compiler in the `PATH` that is different from `CMAKE_C_COMPILER`. That is not what the user requested and we should report that. Please just error out.


================
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:
> > 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.


Repository:
  rL LLVM

https://reviews.llvm.org/D14254