[Clang][OpenMP] libompd: get libomp hwloc includedir by target_link_libraries

[bernhardkaindl]

Summary

When hwloc is used and is installed outside of the default paths, the omp CMake target
needs to provide the needed include path thru the CMake target by adding it with
target_include_directories to it, so libompd gets it as well when it defines it's cmake
target using target_link_libraries.

As suggested in D122667

[ye-luo]

openmp/runtime/src/CMakeLists.txt

158

Please fix
https://github.com/llvm/llvm-project/blob/dd018b96d4cce044feb9edd6b5da59b718231ace/openmp/runtime/cmake/config-ix.cmake#L336
check_include_file(hwloc.h LIBOMP_HAVE_HWLOC_H)
guarantees the hwloc.h file can be found but doesn't mean ${LIBOMP_HWLOC_INSTALL_DIR}/include is a valid path.
Please add before check_include_file
find_path(LIBOMP_HWLOC_INCLUDE_DIR NAMES hwloc.h HINTS ${LIBOMP_HWLOC_INSTALL_DIR} PATH_SUFFIXES include)
to confirm a solid include path to the header file,
and then use LIBOMP_HWLOC_INCLUDE_DIR in target_include_directories here.

[bernhardkaindl]

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()

 if(OPENMP_MSVC_NAME_SCHEME)
   if(uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")

Exactly as suggested. Thanks @ye-luo for the exact blueprint what to do!

[ye-luo]

One minor change requested. All the rest LGTM.

openmp/runtime/cmake/config-ix.cmake

336 ↗

(On Diff #423208)

Please remove line 335 and add after 336
set(CMAKE_REQUIRED_INCLUDES ${LIBOMP_HWLOC_INCLUDE_DIR})

[bernhardkaindl]

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

[ye-luo]

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.

[bernhardkaindl]

In D123888#3455186, @ye-luo wrote:

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.

Great, so your review is finished by this update. The job "Build and test" of the pre-merge check is running now.

[bernhardkaindl]

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?

[ye-luo]

The debian failure seems unrelated. Let me know if you need me to commit the patch.

[bernhardkaindl]

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

openmp/libompd/src/CMakeLists.txt

 
 add_library (ompd SHARED TargetValue.cpp omp-debug.cpp omp-state.cpp omp-icv.cpp)
 
-add_dependencies(ompd omp) # ensure generated import library is created first
+target_link_libraries(ompd omp) # ensure generated import library is created first
 
 set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
 

openmp/runtime/src/CMakeLists.txt

   ${LIBOMP_INC_DIR}
   ${LIBOMP_SRC_DIR}/thirdparty/ittnotify
 )
-if(${LIBOMP_USE_HWLOC})
-  include_directories(${LIBOMP_HWLOC_INSTALL_DIR}/include)
-endif()
 
 # Building with time profiling support requires LLVM directory includes.
 if(LIBOMP_PROFILING_SUPPORT)

   # 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_INSTALL_DIR}/include>"
+                             "$<INSTALL_INTERFACE:${LIBOMP_HWLOC_INSTALL_DIR}/include>"
+  )
+endif()
 
 if(OPENMP_MSVC_NAME_SCHEME)
   if(uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")