User Details
- User Since
- Apr 15 2022, 5:21 PM (11 w, 21 h)
Apr 20 2022
Hi Ye Luo! Yes, I would need you to commit the patch. Thanks and have a good day, Bernhard
Apr 16 2022
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 15 2022
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
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()
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()
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()@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.