Patch to fix build error for libompd due to missing hwloc header location. Full details of build error at https://github.com/spack/spack/issues/29751 and summary available at https://github.com/llvm/llvm-project/issues/54621
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
LIBOMP_INCLUDE_DIR is a weird name because aren't we building libomp? That should be a directory were headers are build/installed not where they are gotten from. For hermetic builds, the distinction of locations to read vs locations to write is very important.
I would prefer a morecomprehensive fix that starts with auditing
$ git grep LIBOMP_INCLUDE_DIR openmp/libompd/src/CMakeLists.txt: ${LIBOMP_INCLUDE_DIR} openmp/libomptarget/CMakeLists.txt:set(LIBOMPTARGET_OPENMP_HEADER_FOLDER "${LIBOMP_INCLUDE_DIR}" CACHE STRING openmp/runtime/CMakeLists.txt:set(LIBOMP_INCLUDE_DIR ${LIBOMP_INCLUDE_DIR} PARENT_SCOPE) openmp/runtime/src/CMakeLists.txt:set(LIBOMP_INCLUDE_DIR ${CMAKE_CURRENT_BINARY_DIR} PARENT_SCOPE) openmp/runtime/src/CMakeLists.txt:set(LIBOMP_INCLUDE_DIR ${CMAKE_CURRENT_BINARY_DIR} PARENT_SCOPE) openmp/tools/archer/CMakeLists.txt: include_directories(${LIBOMP_INCLUDE_DIR}) openmp/tools/archer/tests/lit.site.cfg.in:config.omp_header_dir = "@LIBOMP_INCLUDE_DIR@" openmp/tools/multiplex/CMakeLists.txt: include_directories(${LIBOMP_INCLUDE_DIR}) openmp/tools/multiplex/tests/lit.site.cfg.in:config.omp_header_dir = "@LIBOMP_INCLUDE_DIR@"
to get a better idea of how this variable is currently used and what it is fundamentally for.
Comment Actions
Per this comment at openmp/libomptarget/CMakeLists.txt, LIBOMP_INCLUDE_DIR represents "Path to folder containing omp.h". Note that omp.h is generated from openmp/runtime/src/include/omp.h.var per this recipe. Also, omp.h.var does not seem to include hwloc.h (conditionally or unconditionally).
Looking at the rule for generating omp.h more closely, (which is configure_file(${LIBOMP_INC_DIR}/omp.h.var omp.h @ONLY)), it looks like the ${LIBOMP_INC_DIR}/omp.h.var generates omp.h in the build directory that subsequently gets defined as LIBOMP_INCLUDE_DIR. Thus, any sub-project that is built after the omp.h is generated, uses LIBOMP_INCLUDE_DIR to include the aforementioned header.
I don't know enough about LLVM's OpenMP internals to decide whether all projects that use omp.h need to access hwloc.h (conditionally) or not which the above patch does. If not, we could conditionally include the location ${LIBOMP_HWLOC_INSTALL_DIR}/include in the set of includes for libompd (at openmp/libompd/src/CMakeLists.txt).
Comment Actions
$ git grep hwloc.h
openmp/README.rst: ``hwloc.h`` in ``${LIBOMP_HWLOC_INSTALL_DIR}/include`` and the library in
openmp/runtime/cmake/config-ix.cmake: check_include_file(hwloc.h LIBOMP_HAVE_HWLOC_H)
openmp/runtime/src/kmp.h:#include "hwloc.h"was useful to me.
In openmp/runtime/src/CMakeLists.txt in particular we have
if(${LIBOMP_USE_HWLOC})
include_directories(${LIBOMP_HWLOC_INSTALL_DIR}/include)
endif()The problem is that include_directories just affects the current build, but hwloc.h is being included in a *public* header, right?
The solution is to use target_include_directories (See https://cmake.org/cmake/help/latest/command/target_include_directories.html) with PUBLIC so the include directories are properly propogated downstream.
Comment Actions
@s-sajid-ali Thanks! Your call, but might you want to make the other ones target_include_directories(..., PRIVATE) while you are at it?
Comment Actions
No, it is used for library internals only. And libompd wants to include libomp's internal header, so I am not sure how target_include_directories can apply here.
Current patch does not work. I think the solution would be to add same conditional include_directories to openmp/libompd/src/CMakeLists.txt.
Comment Actions
I meant "simple" solution. Probably target_include_directories can be a "better" solution, but I am not a cmake expert, and don't know how to use target_include_directories. Obviously current usage is wrong.
Comment Actions
The best way to handle include and link is via target_link_libraries (https://cmake.org/cmake/help/latest/command/target_link_libraries.html). This requires to search/find hwloc library in a CMake standard way, and link it properly. Everything else will be handled correctly.
| openmp/runtime/src/CMakeLists.txt | ||
|---|---|---|
| 50 ↗ | (On Diff #419150) | https://cmake.org/cmake/help/latest/command/target_include_directories.html |
This comment was removed by Ericson2314.
Comment Actions
@tianshilei1992 is right. Basically the older CMake stuff like include_directories only worked by side effects, and therefore is impossible to use correctly and modulary and is just generally terrible. the new target stuff is much better, and actually allows tracking who depends on what.
target_link_libraries is definitely a good idea, but I not trying to force @s-sajid-ali to fix the world just to fix the bug :). If we can just get away with target_include_directories that is a good initial step.
@AndreyChurbanov Can you explain
No, it is used for library internals only. And libompd wants to include libomp's internal header, so I am not sure how target_include_directories can apply here.
in a bit more detail? I know CMake well, but not the LLVM openmp stuff.
Is kmp.h that includes hwloc.h installed or not? If it is installed, then what @s-sajid-ali is correct even if the header is not really designed for external use. If it is not installed, then we will have to be more careful about what "target" to use.
Comment Actions
Looks like kmp,h is not installed. OK this will call for something fancier.
Look for add_custom_target(libomp-needed-headers
My suggestion would be to
- make add_custom_target(libomp-kmp-headers)
- target_include_directories(libomp-kmp-headers ${LIBOMP_HWLOC_INSTALL_DIR}/include PUBLIC)
- make ompd and omp depend on libomp-kmp-headers PRIVATRELY
good luck!
Comment Actions
The kmp.h is not installed, it is used only for library build. And hwloc library is used by libomp dynamically only if it is accessible at runtime, it is not "linked" with libomp.
I actually was able to use target_include_directories, but this needs more changes - at least it needs to specify target which is not yet set at the current place of CMakeLists.txt. And then the problem of libompd compilation is not fixed by this, the target_include_directories will need to also be added to openmp/libompd/src/CMakeLists.txt anyway.
So the "simple" solution as I said - addition of include_directories into openmp/libompd/src/CMakeLists.txt. The solution with target_include_directories needs more files restructuring.
Comment Actions
Sorry, I was in a hurry:(. Looks like the libomp depends on libhwloc when LIBOMP_USE_HWLOC is true. So @tianshilei is right, we would better use target_link_libraries. Only I am not sure it worth doing in this patch.
| openmp/runtime/src/CMakeLists.txt | ||
|---|---|---|
| 50 ↗ | (On Diff #419150) | Not sure if PUBLIC is the right one here. If something else depends on omp, does the header need to be propagated? |
Comment Actions
Adopt the simple approach (as suggested by @AndreyChurbanov) to fix the bug for now. Perhaps someone with more extensive knowledge of LLVM's OpenMP implementation and CMake can perform a more comprehensive fix in the future.
Comment Actions
We can land this simple one now, but I really don't like doing stop-gap fixes on development branches instead of fixing the underlying issue. The fact of the matter is, if tech debt isn't fixed as prompted by bugs, it usually never gets fixed at all.
I don't think target_link_libraries effects headers at all, so while it is good it is a big of a red herring.
The instructions I gave above with add_custom_target(libomp-kmp-headers) should work.
Comment Actions
No, it does. If the CMake target is created properly, it will have target_include_directories with INTERFACE, and then the include directories will be propagated if someone else links the library.
Comment Actions
Hmm. Do you have docs for this, I would like for you to be right? https://cmake.org/cmake/help/latest/command/target_link_libraries.html only mentions macOS framework includes.
Comment Actions
https://cmake.org/cmake/help/latest/command/target_include_directories.html
The INTERFACE, PUBLIC and PRIVATE keywords are required to specify the scope of the following arguments. PRIVATE and PUBLIC items will populate the INCLUDE_DIRECTORIES property of <target>. PUBLIC and INTERFACE items will populate the INTERFACE_INCLUDE_DIRECTORIES property of <target>. The following arguments specify include directories.
This feature is actively used in libomptarget. For example, in openmp/libomptarget/plugins/cuda/CMakeLists.txt the target omptarget.rtl.cuda depends on elf_common, which is defined in openmp/libomptarget/plugins/common/elf_common/CMakeLists.txt, but we don't set include directory for omptarget.rtl.cuda to include headers of elf_common.
Comment Actions
@tianshilei1992 Oh sorry you were talking about https://cmake.org/cmake/help/latest/prop_tgt/INTERFACE_INCLUDE_DIRECTORIES.html#prop_tgt:INTERFACE_INCLUDE_DIRECTORIES that behavior right? Specific to when target_link_library is used to depend on a target and not, say, an external preexisting library file.
So the
add_dependencies(ompd omp)
need to be replaced with a target_link_libraries(ompd omp), right?
Comment Actions
Also, how about all this code goes to kmp_affinity.h? Then we dodge the problem entirely.
The kmp_hwloc_depth_t typedef is also now dead code since 9982f33e2c3aeba0b59555286d0d1e99697c62a0 and can be removed.
Comment Actions
Right. As long as omp is set properly, target_link_libraries can set everything else automatically.
Comment Actions
@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.
The reason is that the hwloc include path is not exported by the library, and thus cannot be picked up by target_link_libraries. Here is the part of the cmake documentation which explains what is needed to make this work:
https://cmake.org/cmake/help/git-stage/guide/importing-exporting/index.html
And then use the target_include_directories() command to specify the include directories for the target:
target_include_directories(MathFunctions
PUBLIC
"$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>"
"$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>"
)In this case, the "$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>" part should be enough.
It has to be checked, if the minimum cmake requried by openmp for llvm-14/15 (3.13.4 in the llvm-14 source) supports this method.
This variant should work with old cmake versions as well:
target_include_directories(omp INTERFACE "${HWLOC_INC_PATH}Comment Actions
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()
# Building with time profiling support requires LLVM directory includes.
if(LIBOMP_PROFILING_SUPPORT)
include_directories(
${LLVM_MAIN_INCLUDE_DIR}
${LLVM_INCLUDE_DIR}
)
endif()To export ${LIBOMP_HWLOC_INSTALL_DIR}/include as an INTERFACE of the omp target, this needs to be modernized (Using include_directories() is very old cmake style) and applied to the omp target so it is exported as an interface with the omp target.
Comment Actions
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() # Building with time profiling support requires LLVM directory includes. if(LIBOMP_PROFILING_SUPPORT) @@ -157,6 +154,13 @@ # 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) --- openmp/libompd/src/CMakeLists.txt +++ openmp/libompd/src/CMakeLists.txt @@ -13,7 +13,7 @@ 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")
This (by comparison) relatively modern cmake style is used in the llvm pstl and mlir libraries already, It is needed to attach the include path to the omp CMake target, so target_link_libraries can pick it up from the target to use ompis used by another target.
Comment Actions
Thanks for the comment. Yes, target_link_libraries can only work if the libraries are properly configured. Now it looks like libomp is not configured properly.
The good news is, based on https://discourse.llvm.org/t/rfc-stand-alone-build-support/, if we want to use external library, we have to reply on CMake instead of manual configured twisted CMake commands. It's about time to modernize the whole CMake structure of OpenMP, but of course it's beyond the scope of this patch.
Comment Actions
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.
Comment Actions
The dependency at the build failure is:
openmp/libompd/src/omp-icv.cpp:21:
-> openmp/runtime/src/kmp.h:96:
--> #include "hwloc.h"
The size of struct dispatch_shared_info / dispatch_shared_info_t depends on if #KMP_USE_HWLOC is set, from openmp/runtime/src/kmp.h
#if KMP_USE_HWLOC // When linking with libhwloc, the ORDERED EPCC test slows down on big // machines (> 48 cores). Performance analysis showed that a cache thrash // was occurring and this padding helps alleviate the problem. char padding[64]; #endif } dispatch_shared_info_t;
ompd does not appear to use hwloc_* symbols directly, but I'd say risking to break code just to avoid the include path in libompd does not appear to be worth any breakage.
I uploaded my revision as https://reviews.llvm.org/D123888 and it has been further improved and accepted.