[Openmp-commits] [PATCH] D55725: [OpenMP] Add libs to clang-dedicated directories

[Hal Finkel via Phabricator via Openmp-commits]

hfinkel added inline comments.


================
Comment at: openmp/CMakeLists.txt:55
+endif()
+
 # Check and set up common compiler flags.
----------------
jdenny wrote:
> hfinkel wrote:
> > The way this is computed looks different to me than what I believe to be the corresponding code in libc++'s file. It's this, right?
> > 
> >   string(REGEX MATCH "[0-9]+\\.[0-9]+(\\.[0-9]+)?" CLANG_VERSION
> >          ${PACKAGE_VERSION})
> >   
> >   if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
> >     set(DEFAULT_INSTALL_PREFIX lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/${LLVM_DEFAULT_TARGET_TRIPLE}/)
> >     set(DEFAULT_INSTALL_HEADER_PREFIX lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/)
> >     set(LIBCXX_LIBRARY_DIR 
> >   ${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION}/${LLVM_DEFAULT_TARGET_TRIPLE}/lib${LIBCXX_LIBDIR_SUFFIX})
> >     set(LIBCXX_HEADER_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION})
> >   elseif(LLVM_LIBRARY_OUTPUT_INTDIR)
> >     set(LIBCXX_LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
> >     set(LIBCXX_HEADER_DIR  ${LLVM_BINARY_DIR})
> >   else()
> >     set(LIBCXX_LIBRARY_DIR ${CMAKE_BINARY_DIR}/lib${LIBCXX_LIBDIR_SUFFIX})
> >   endif()
> > 
> > 
> Thanks for reviewing.  It seems like each subproject has a slightly different formula.
> 
> What this patch does here is similar to what's in `compiler-rt/cmake/base-config-ix.cmake` except this patch doesn't attempt to observe `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR`.  Perhaps it should?
> 
> Below is some of the relevant code from that script (I've omitted many seemingly irrelevant lines):
> 
> ```
> if (LLVM_TREE_AVAILABLE)
>   set(COMPILER_RT_OUTPUT_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION})
>   set(COMPILER_RT_INSTALL_PATH lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION})
> else()
>   set(COMPILER_RT_OUTPUT_DIR ${CMAKE_CURRENT_BINARY_DIR} CACHE PATH
>     "Path where built compiler-rt libraries should be stored.")
>   set(COMPILER_RT_INSTALL_PATH ${CMAKE_INSTALL_PREFIX} CACHE PATH
>     "Path where built compiler-rt libraries should be installed.")
> endif()
> 
> if(NOT DEFINED COMPILER_RT_OS_DIR)
>   string(TOLOWER ${CMAKE_SYSTEM_NAME} COMPILER_RT_OS_DIR)
> endif()
> if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
>   set(COMPILER_RT_LIBRARY_OUTPUT_DIR
>     ${COMPILER_RT_OUTPUT_DIR})
>   set(COMPILER_RT_LIBRARY_INSTALL_DIR
>     ${COMPILER_RT_INSTALL_PATH})
> else(LVM_ENABLE_PER_TARGET_RUNTIME_DIR)
>   set(COMPILER_RT_LIBRARY_OUTPUT_DIR
>     ${COMPILER_RT_OUTPUT_DIR}/lib/${COMPILER_RT_OS_DIR})
>   set(COMPILER_RT_LIBRARY_INSTALL_DIR
>     ${COMPILER_RT_INSTALL_PATH}/lib/${COMPILER_RT_OS_DIR})
> endif()
> ```
> 
> ... It seems like each subproject has a slightly different formula.

I can certainly believe that.

> What this patch does here is similar to what's in compiler-rt/cmake/base-config-ix.cmake 

SGTM.

> except this patch doesn't attempt to observe LLVM_ENABLE_PER_TARGET_RUNTIME_DIR. Perhaps it should?

Yes, I think that it should.



Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D55725