[Openmp-commits] [PATCH] D114083: [OpenMP] Disable libomptarget profiling by default if built via the "runtimes" setup

[Martin Storsjö via Phabricator via Openmp-commits]

mstorsjo added inline comments.


================
Comment at: openmp/CMakeLists.txt:60
 # there is no point in trying to compile libomptarget on other OSes.
 if (APPLE OR WIN32 OR NOT OPENMP_HAVE_STD_CPP14_FLAG)
   set(ENABLE_LIBOMPTARGET OFF)
----------------
protze.joachim wrote:
> For the cross-compilation use case, shouldn't the question be whether the code is compiled for Windows/MacOS rather than compiled on Windows/MacOS?
In CMake, `APPLE` and `WIN32` refer to the target of the current cross compiler, not the host running the compilation. So you can do e.g. `if (WIN32) # add windows specific sources endif()` which is safe and correct wrt cross compilation.


================
Comment at: openmp/CMakeLists.txt:65
+set(ENABLE_LIBOMPTARGET_PROFILING OFF)
+if (ENABLE_LIBOMPTARGET AND NOT LLVM_RUNTIMES_BUILD)
+  set(ENABLE_LIBOMPTARGET_PROFILING ON)
----------------
protze.joachim wrote:
> Can we check, whether the target architectures for llvm and runtimes are the same? In that case, building with profiling sounds reasonable.
I guess we could, but I'm afraid it could become brittle.

The situation that prompted this was that the main LLVM build and the runtimes disagreed on whether zlib was found, even if they were targeting the exact same OS/architecture. (They disagreed because the runtimes build used/will use a slightly different cmake setup, which makes the library probing behave slightly differently - also in general, runtime libraries shouldn't need to probe for external dependencies). As the main LLVM setup had zlib (which was a dependency of LLVMSupport) but the runtimes build didn't, libopenmptarget failed to include the LLVMSupport dependency.

I'm just suggesting changing the default here - one can still request it enabled (and see if it actually works or not).

The main issue as I see it, is that when building via the runtimes setup, using LLVMSupport from a runtime is a bit of a layering violation/inversion - the main overall direction is to untangle the runtimes from the main LLVM build as much as possible, that's at least what @phosek has suggested.

If building via `LLVM_ENABLE_PROJECTS`, so that the openmp runtime is built with the same compiler and configuration, alongside LLVM itself, using LLVMSupport is fine. Although, at least libcxx/libcxxabi/libunwind are entirely deprecating building them that way, but I'm not sure if openmp has quite as much desire to deprecate that setup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114083