Page MenuHomePhabricator

bernhardkaindl (Bernhard Kaindl)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 15 2022, 5:21 PM (11 w, 21 h)

Recent Activity

Apr 20 2022

bernhardkaindl added a comment to D123888: [Clang][OpenMP] libompd: get libomp hwloc includedir by target_link_libraries.

Hi Ye Luo! Yes, I would need you to commit the patch. Thanks and have a good day, Bernhard

Apr 20 2022, 8:54 AM · Restricted Project, Restricted Project

Apr 16 2022

bernhardkaindl added a comment to D123888: [Clang][OpenMP] libompd: get libomp hwloc includedir by target_link_libraries.

Debian is broken for some unrelated reason, Libcxx CI has passed: https://buildkite.com/llvm-project/libcxx-ci/builds/1024 - Status is Ready to land, btw. What next?

Apr 16 2022, 3:42 PM · Restricted Project, Restricted Project

Apr 15 2022

bernhardkaindl added a comment to D123888: [Clang][OpenMP] libompd: get libomp hwloc includedir by target_link_libraries.

check_include_file is like writing an empty main with one include line and gets it compiled. So it doesn't pull any dependency.
To test whether hwloc.h is a valid header file, you'd need to provide CMAKE_REQUIRED_INCLUDES pointing to hwloc.h.
So it is needed.

Apr 15 2022, 8:41 PM · Restricted Project, Restricted Project
bernhardkaindl updated the diff for D123888: [Clang][OpenMP] libompd: get libomp hwloc includedir by target_link_libraries.

Looks perfect now! Thanks @ye-luo, incremental diff:

--- openmp/runtime/cmake/config-ix.cmake.2
+++ openmp/runtime/cmake/config-ix.cmake.3
@@ -333,6 +333,6 @@
 # Check if HWLOC support is available
 if(${LIBOMP_USE_HWLOC})
-  set(CMAKE_REQUIRED_INCLUDES ${LIBOMP_HWLOC_INSTALL_DIR}/include)
   find_path(LIBOMP_HWLOC_INCLUDE_DIR NAMES hwloc.h HINTS ${LIBOMP_HWLOC_INSTALL_DIR} PATH_SUFFIXES include)
+  set(CMAKE_REQUIRED_INCLUDES ${LIBOMP_HWLOC_INCLUDE_DIR})
   check_include_file(hwloc.h LIBOMP_HAVE_HWLOC_H)
   set(CMAKE_REQUIRED_INCLUDES)

I believe we would not even need the new line 336

set(CMAKE_REQUIRED_INCLUDES ${LIBOMP_HWLOC_INCLUDE_DIR})

anymore because of "$<BUILD_INTERFACE:${LIBOMP_HWLOC_INCLUDE_DIR}>", here, but if in doubt, the current revision LGTM too.

--- a/openmp/runtime/src/CMakeLists.txt
+++ b/openmp/runtime/src/CMakeLists.txt
@@ -157,6 +154,13 @@ else()
   # libomp must be a C++ library such that it can link libLLVMSupport
   set(LIBOMP_LINKER_LANGUAGE CXX)
 endif()
+if(${LIBOMP_USE_HWLOC})
+  target_include_directories(omp
+                             PUBLIC
+                            "$<BUILD_INTERFACE:${LIBOMP_HWLOC_INCLUDE_DIR}>"
+                            "$<INSTALL_INTERFACE:${LIBOMP_HWLOC_INCLUDE_DIR}>"
+  )
+endif()

I test using a build of llvm (using ninja) and didn't test a standalone build of just openmp

Apr 15 2022, 8:33 PM · Restricted Project, Restricted Project
bernhardkaindl updated the diff for D123888: [Clang][OpenMP] libompd: get libomp hwloc includedir by target_link_libraries.

Thanks, added:

--- a/openmp/runtime/cmake/config-ix.cmake
+++ b/openmp/runtime/cmake/config-ix.cmake
@@ -333,6 +333,7 @@ endif()
 # Check if HWLOC support is available
 if(${LIBOMP_USE_HWLOC})
   set(CMAKE_REQUIRED_INCLUDES ${LIBOMP_HWLOC_INSTALL_DIR}/include)
+  find_path(LIBOMP_HWLOC_INCLUDE_DIR NAMES hwloc.h HINTS ${LIBOMP_HWLOC_INSTALL_DIR} PATH_SUFFIXES include)
   check_include_file(hwloc.h LIBOMP_HAVE_HWLOC_H)
   set(CMAKE_REQUIRED_INCLUDES)
   find_library(LIBOMP_HWLOC_LIBRARY

And use the created LIBOMP_HWLOC_INCLUDE_DIR here:

--- a/openmp/runtime/src/CMakeLists.txt
+++ b/openmp/runtime/src/CMakeLists.txt
@@ -157,6 +154,13 @@ else()
   # libomp must be a C++ library such that it can link libLLVMSupport
   set(LIBOMP_LINKER_LANGUAGE CXX)
 endif()
+if(${LIBOMP_USE_HWLOC})
+  target_include_directories(omp
+                             PUBLIC
+          "$<BUILD_INTERFACE:${LIBOMP_HWLOC_INCLUDE_DIR}>"
+          "$<INSTALL_INTERFACE:${LIBOMP_HWLOC_INCLUDE_DIR}>"
+  )
+endif()
Apr 15 2022, 8:10 PM · Restricted Project, Restricted Project
bernhardkaindl added a comment to D122667: llvm14 patch: hwloc include directory for libompd.

An add-up to this patch is, if both ompd and omp depend on hwloc (like including hwloc headers`), both should set target_link_libraries. The dependence should be:

omp---->hwloc
         ^
ompd-----|

The transitive dependence is bad practice.

Apr 15 2022, 7:48 PM · Restricted Project, Restricted Project
bernhardkaindl updated the summary of D123888: [Clang][OpenMP] libompd: get libomp hwloc includedir by target_link_libraries.
Apr 15 2022, 7:18 PM · Restricted Project, Restricted Project
bernhardkaindl requested review of D123888: [Clang][OpenMP] libompd: get libomp hwloc includedir by target_link_libraries.
Apr 15 2022, 7:16 PM · Restricted Project, Restricted Project
bernhardkaindl added a comment to D122667: llvm14 patch: hwloc include directory for libompd.

This is the minimum fix for exporting ${LIBOMP_HWLOC_INSTALL_DIR}/include thru the omp target for import thru target_link_libraries.

--- openmp/runtime/src/CMakeLists.txt
+++ openmp/runtime/src/CMakeLists.txt
@@ -46,9 +46,6 @@
   ${LIBOMP_INC_DIR}
   ${LIBOMP_SRC_DIR}/thirdparty/ittnotify
 )
-if(${LIBOMP_USE_HWLOC})
-  include_directories(${LIBOMP_HWLOC_INSTALL_DIR}/include)
-endif()
Apr 15 2022, 6:19 PM · Restricted Project, Restricted Project
bernhardkaindl added a comment to D122667: llvm14 patch: hwloc include directory for libompd.

I just took a closer look to fix this and found this in openmp/runtime/src/CMakeLists.txt

# Set the -I includes for all sources
include_directories(
  ${CMAKE_CURRENT_BINARY_DIR}
  ${LIBOMP_SRC_DIR}
  ${LIBOMP_SRC_DIR}/i18n
  ${LIBOMP_INC_DIR}
  ${LIBOMP_SRC_DIR}/thirdparty/ittnotify
)
if(${LIBOMP_USE_HWLOC})
  include_directories(${LIBOMP_HWLOC_INSTALL_DIR}/include)
endif()
Apr 15 2022, 5:52 PM · Restricted Project, Restricted Project
bernhardkaindl requested changes to D122667: llvm14 patch: hwloc include directory for libompd.

@s-sajid-ali Only replacing add_dependencies(ompd omp) with target_link_libraries(ompd omp) is not enough to fix the include path. I just tested, it fails to build.

Apr 15 2022, 5:41 PM · Restricted Project, Restricted Project