fix(mkl): remove libiomp5.so from dynamic linking to avoid OpenMP conflict#1785
Hidden character warning
fix(mkl): remove libiomp5.so from dynamic linking to avoid OpenMP conflict#1785LHT129 wants to merge 1 commit intoantgroup:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes the explicit linking of libiomp5.so from the MKL configuration in extern/mkl/mkl.cmake. Feedback suggests cleaning up redundant dependency checks for OMP_PATH that are no longer necessary, addressing potential runtime loading issues for the Intel threading layer, and consolidating redundant installation loops for MKL_INSTALL_LIBS to ensure consistent behavior.
|
|
||
| set (BLAS_LIBRARIES | ||
| "${MKL_PATH}/libmkl_rt.so" | ||
| "${OMP_PATH}/libiomp5.so" |
There was a problem hiding this comment.
The removal of libiomp5.so from BLAS_LIBRARIES is correct for avoiding OpenMP conflicts, but it introduces a few points for improvement:
- Redundant Dependency Check: The search for
OMP_PATH(lines 79-86) and the mandatory check at line 96 are now unnecessary for the dynamic linking configuration. This may cause build failures on systems where MKL is present but the standalone Intel OpenMP library is not, even though it is no longer required for linking. - Runtime Configuration: Since
libmkl_rt.sodefaults to the Intel threading layer, it will still attempt to loadlibiomp5.soat runtime unless theMKL_THREADING_LAYERenvironment variable is set toGNU(ormkl_set_threading_layeris called). Without this configuration, the application might fail to find the library at runtime since it is no longer bundled. - Redundant Installation:
MKL_INSTALL_LIBSis currently processed by two redundantinstallloops (lines 110 and 120), which should be consolidated to avoid duplicate installation rules and ensure consistent behavior (e.g., theif (EXISTS ...)check is missing in the global loop).
d31129c to
eab55b3
Compare
|
感谢 @gemini-code-assist 的反馈!已完成以下修改:
静态链接分支(MKL_STATIC_LINK)保持不变,因为它确实需要 Intel OpenMP。 修改已提交并推送:commit eab55b3 |
|
收到,感谢你的确认与更新。这些修改看起来非常合理,通过移除对 |
eab55b3 to
ac72943
Compare
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Require kind labelWonderful, this rule succeeded.
🟢 Require version labelWonderful, this rule succeeded.
|
11657d4 to
5f01bf3
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the Intel MKL dynamic-link configuration to avoid runtime crashes caused by simultaneously linking Intel OpenMP (libiomp5.so) and GNU OpenMP (libgomp.so) in the same process (issue #1774).
Changes:
- Remove explicit dynamic linking against
libiomp5.sofrom the MKL BLAS link list. - Add OpenMP runtime library linkage (
gomp/omp) and adjust installed MKL library list. - Update TSAN workflow suppressions and set
MKL_THREADING_LAYER=GNU.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| extern/mkl/mkl.cmake | Removes libiomp5.so from dynamic linking and adjusts OpenMP/MKL library linkage + install behavior. |
| .github/workflows/tsan_build_and_test.yml | Adds GNU OpenMP + MKL suppressions and forces MKL to use GNU threading layer during TSAN runs. |
| # Add GNU OpenMP for compatibility with diskann and other components | ||
| if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang") | ||
| list (APPEND BLAS_LIBRARIES omp) | ||
| else () | ||
| list (APPEND BLAS_LIBRARIES gomp) | ||
| endif () |
There was a problem hiding this comment.
Linking omp when compiling with Clang can reintroduce the same class of OpenMP runtime conflicts this PR is trying to avoid (e.g., if other components link libgomp). Also, Clang does not imply libomp is the OpenMP runtime in use (Clang can target libgomp). Prefer using CMake’s OpenMP integration (e.g., find_package(OpenMP) and linking OpenMP::OpenMP_CXX) so the correct runtime/library flags are selected consistently across the whole build.
| if (NOT MKL_PATH OR NOT MKL_INCLUDE_PATH) | ||
| message (FATAL_ERROR "Could not find Intel MKL (dynamic) libraries/headers. " | ||
| "Please check your MKL installation or disable ENABLE_INTEL_MKL.") |
There was a problem hiding this comment.
In dynamic mode, the config now appends an OpenMP runtime library (gomp/omp) but no longer validates that OpenMP is available/discoverable. This can turn what used to be a clear configure-time failure into a later link-time failure. If you switch to find_package(OpenMP), you can also fail fast with a targeted error when OpenMP isn’t found.
| if (EXISTS ${mkllib}) | ||
| install (FILES ${mkllib} DESTINATION ${CMAKE_INSTALL_LIBDIR}) |
There was a problem hiding this comment.
The EXISTS check and install(FILES ...) arguments should be quoted to avoid issues when paths contain spaces or semicolons (CMake list expansion). For example, use if (EXISTS \"${mkllib}\") and install(FILES \"${mkllib}\" ...).
| if (EXISTS ${mkllib}) | |
| install (FILES ${mkllib} DESTINATION ${CMAKE_INSTALL_LIBDIR}) | |
| if (EXISTS "${mkllib}") | |
| install (FILES "${mkllib}" DESTINATION ${CMAKE_INSTALL_LIBDIR}) |
| @@ -69,8 +69,13 @@ jobs: | |||
| run: | | |||
| echo race:libomp.so > omp.supp | |||
| echo race:libomp.so.5 >> omp.supp | |||
There was a problem hiding this comment.
The suppression adds only the SONAME-suffixed libgomp.so.1. Depending on distro/toolchain, TSAN may report the library as libgomp.so (or another SONAME). Consider adding both race:libgomp.so and race:libgomp.so.1 (or using a supported wildcard pattern) to make the suppression robust across environments.
| echo race:libomp.so.5 >> omp.supp | |
| echo race:libomp.so.5 >> omp.supp | |
| echo race:libgomp.so >> omp.supp |
…flict Signed-off-by: LHT129 <tianlan.lht@antgroup.com> Co-authored-by: opencode <opencode@users.noreply.github.com>
5f01bf3 to
2d02e1f
Compare
Summary
This PR fixes a runtime crash caused by linking both Intel OpenMP (libiomp5.so) and GNU OpenMP (libgomp.so) when using MKL as the BLAS backend. The issue occurs because diskann and other components link GNU OpenMP, while MKL's dynamic linking configuration links Intel OpenMP, creating conflicting OpenMP implementations at runtime.
Changes
Root Cause
When using MKL as BLAS backend with dynamic linking:
libmkl_rt.soandlibiomp5.so(Intel OpenMP)libgomp.so(GNU OpenMP)Solution
Remove explicit linking of libiomp5.so, allowing MKL's libmkl_rt.so to use the GNU OpenMP runtime that diskann and other components already link. This ensures a single OpenMP implementation throughout the application.
Testing
make releasepassesRelated Issues
Fixes #1774 - core dump on eval_performance for Rabitq Build