[Openmp-commits] [PATCH] D101509: An attempt to abandon omptarget out-of-tree builds.

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt:27
+  # 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}
----------------
vzakhari wrote:
> tianshilei1992 wrote:
> > tianshilei1992 wrote:
> > > vzakhari wrote:
> > > > tianshilei1992 wrote:
> > > > > This line of code is problematic. If I build OpenMP standalone and set `CMAKE_C_COMPILER` to the right `clang`, and the right `clang` is not in `$PATH`, it still finds the one in `$PATH`.
> > > > I am not sure I understand the problem.  These `find_program` invocations explicitly look the tools (including `clang`) in `${LLVM_TOOLS_BINARY_DIR}`, which is supposed to be set up by `find_package` (which also sets `LLVM_DIR` the causes cmake to hit this clause).  Is it the case that in your build you get valid `LLVM_DIR` and invalid `LLVM_TOOLS_BINARY_DIR`?  Is it possible that your pre-installed LLVM package is corrupted?
> > > > 
> > > > Can you please insert `libomptarget_say("Using: ${LLVM_DIR}; using ${LLVM_TOOLS_BINARY_DIR}")` here and tell me what it is printing?
> > > Okay. I see the problem here. This patch was trying to abandon out-of-tree build of `libomptarget`, which exactly what I'm using. So the root cause is, I have multiple version of LLVM installed in my system. `find_package(LLVM)` can return one of them, but not the one I want to use (I cannot control the system so I cannot remove those LLVMs in my system). As a result, `LLVM_TOOLS_BINARY_DIR` is set to the wrong place, therefore the `clang` detected is also wrong, no matter what compiler I set for CMake. So in order to let CMake pick up the one I want, `LLVM_ROOT` needs to be set when invoking CMake.
> > So actually, even with `find_package(LLVM)`, CMake can still pick up a wrong version. That's why I guard it with a version in https://reviews.llvm.org/D102587.
> Thank you for the explanation.  Can you please clarify what you mean by "a wrong version"?  Is there any code in `libomptarget` or `plugins` that requires some minimal `LLVM` version?  In other words, what kind of a breakage you get in your build?
`libomptarget` uses some LLVM functions that they were in a different form compared with current one. My system has LLVM 8. If there is no version constraint, LLVM 8 can be picked up, and its headers will be used for compilation. However, the function signatures have already changed, leading to a compilation error.

It looks like we are using the latest version of LLVM. From my perspective, it's fine to assume it is 12 for now. In the future, once those functions are changed, the pre check in test will fail, which requires the author to update OpenMP code as well. We can revise the version later.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101509/new/

https://reviews.llvm.org/D101509