[LAPACK] Dynamically load reference lapack library for tests. #701
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…conflicting symbols
…ry to find cblas when lapacke_root is given
andrewtbarker
approved these changes
Jul 18, 2025
There was a problem hiding this comment.
Thanks for the changes! I only have some minor comments.
One more thing is that the original inconsistency (reference_blas_templates.hpp in BLAS and lapack_reference_wrappers.hpp in LAPACK) now leads to:
reference_blas_templates.hppincludesreference_blas_wrappers.hppin BLASlapack_reference_wrappers.hppincludeslapack_reference_templates.hppin LAPACK
I will leave this for @andrewtbarker to decide if we need alignment.
| list(REMOVE_ITEM TEST_TARGET_DOMAINS "blas") | ||
| if ("lapack" IN_LIST TEST_TARGET_DOMAINS) |
There was a problem hiding this comment.
Could you please check if it can work without this condition?
| @@ -30,6 +30,11 @@ set(EXAMPLE_TARGET example_lapack_getrs_usm_mklcpu_cusolver) | |||
| # Create a CMake target with one source file | |||
| add_executable(${EXAMPLE_TARGET} getrs_usm_mklcpu_cusolver.cpp) | |||
|
|
|||
| # May be needed to compile source files with SYCL kernels | |||
| if (USE_ADD_SYCL_TO_TARGET_INTEGRATION) | |||
There was a problem hiding this comment.
nit
Suggested change
| if (USE_ADD_SYCL_TO_TARGET_INTEGRATION) | |
| if(USE_ADD_SYCL_TO_TARGET_INTEGRATION) |
| # External applications should use find_package or FetchContent to include oneMath first. | ||
| # See https://github.com/uxlfoundation/oneMath/blob/develop/docs/using_onemath_with_cmake.rst | ||
|
|
||
| # Create a CMake target with one source file | ||
| add_executable(${EXAMPLE_TARGET} getrs_usm.cpp) | ||
|
|
||
| # May be needed to compile source files with SYCL kernels | ||
| if (USE_ADD_SYCL_TO_TARGET_INTEGRATION) |
There was a problem hiding this comment.
nit
Suggested change
| if (USE_ADD_SYCL_TO_TARGET_INTEGRATION) | |
| if(USE_ADD_SYCL_TO_TARGET_INTEGRATION) |
| @@ -86,7 +86,11 @@ if(BUILD_SHARED_LIBS) | |||
| PUBLIC ${CMAKE_BINARY_DIR}/bin | |||
| $<$<BOOL:${LAPACKE_FOUND}>:${LAPACKE_INCLUDE}> | |||
| ) | |||
| target_link_libraries(lapack_source_rt PUBLIC ONEMATH::SYCL::SYCL) | |||
| if (USE_ADD_SYCL_TO_TARGET_INTEGRATION) | |||
There was a problem hiding this comment.
nit
Suggested change
| if (USE_ADD_SYCL_TO_TARGET_INTEGRATION) | |
| if(USE_ADD_SYCL_TO_TARGET_INTEGRATION) |
Comment on lines
+106
to
+107
| if (USE_ADD_SYCL_TO_TARGET_INTEGRATION) | ||
| add_sycl_to_target(TARGET lapack_source_ct SOURCES ${LAPACK_SOURCES}) |
There was a problem hiding this comment.
nit
Suggested change
| if (USE_ADD_SYCL_TO_TARGET_INTEGRATION) | |
| add_sycl_to_target(TARGET lapack_source_ct SOURCES ${LAPACK_SOURCES}) | |
| if(USE_ADD_SYCL_TO_TARGET_INTEGRATION) | |
| add_sycl_to_target(TARGET lapack_source_ct SOURCES ${LAPACK_SOURCES}) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Lapack tests suffered same issue as BLAS tests which was reported in #204 and fixed in #251 . If a backend uses the same LAPACKE interface and symbols as the reference LAPACK, it will end up comparing only the first found library to itself.
This basically copies #251 solution by using dlopen each time the reference library symbols are needed instead of linking to the library directly.
In the process of testing this, I noticed that Lapack tests were not exactly working if only the lapack backend is built, as it depends on the BLAS library as well for accuracy checks. A second commit was added to make Lapack tests work alone, by checking that BLAS reference library can be found even if BLAS backend is not built. It also adds REF_LAPACK_ROOT in the search path for BLAS library, as in the reference design all blas/lapack libraries are in the same location (this avoids having to provide both REF_LAPACK_ROOT and REF_BLAS_ROOT in this case).
100% tests passed, 0 tests failed out of 796