[OpenMP] Add libomp unit test infrastructure #168063
Conversation
ro-i
force-pushed
the
users/ro-i/check-libomp-unit
branch
from
4c9e176 to
c801cb4
Compare
ro-i
force-pushed
the
users/ro-i/check-libomp-unit
branch
from
2f30670 to
b12ff59
Compare
| set_target_properties(OpenMPUnitTests PROPERTIES FOLDER "OpenMP/Tests") | ||
|
|
||
| if(WIN32 OR STUBS_LIBRARY) | ||
| message(WARNING "OpenMP unittests disabled due to stub library or Windows") |
There was a problem hiding this comment.
Any reason this doesn't work on Windows?
There was a problem hiding this comment.
I don't think so. But since the CMake changes are already complicated enough to review and test as they are, I thought it might be best to get things working first and then add something like the Windows support in a follow-up
Co-authored-by: Shilei Tian <[email protected]>
jplehr
left a comment
jplehr
left a comment
There was a problem hiding this comment.
+1 for adding unit tests here!
| target_link_libraries(omp_testing PUBLIC default_gtest) | ||
| target_link_libraries(omp_testing PUBLIC ${LIBOMP_TEST_LIBFLAGS} ${LIBOMP_DL_LIBS}) | ||
| set_target_properties(omp_testing PROPERTIES | ||
| PREFIX "" SUFFIX "" OUTPUT_NAME "libomp_testing${LIBOMP_LIBRARY_SUFFIX}" |
There was a problem hiding this comment.
| PREFIX "" SUFFIX "" OUTPUT_NAME "libomp_testing${LIBOMP_LIBRARY_SUFFIX}" | |
| PREFIX "" | |
| SUFFIX "" | |
| OUTPUT_NAME "libomp_testing${LIBOMP_LIBRARY_SUFFIX}" |
Why override SUFFIX/PREFIX? Would
PREFIX ""
SUFFIX "${LIBOMP_LIBRARY_SUFFIX}"
get the same result?
There was a problem hiding this comment.
I just used
llvm-project/openmp/runtime/src/CMakeLists.txt
Lines 233 to 237 in a3b7989
| PREFIX "" SUFFIX "" OUTPUT_NAME "libomp_testing${LIBOMP_LIBRARY_SUFFIX}" | ||
| LINK_FLAGS "${LIBOMP_TEST_LDFLAGS}" | ||
| LINKER_LANGUAGE ${LIBOMP_LINKER_LANGUAGE} | ||
| EXCLUDE_FROM_ALL TRUE # Don't build by default, only when tests need it |
There was a problem hiding this comment.
| EXCLUDE_FROM_ALL TRUE # Don't build by default, only when tests need it |
EXCLUDE_FROM_ALL can be also just part of add_library:
add_library(omp_testing ... EXCLUDE_FROM_ALL)
There was a problem hiding this comment.
add_library(omp_testing SHARED $<TARGET_OBJECTS:obj.omp> EXCLUDE_FROM_ALL)
whould lead to
CMake Error at .../llvm-project/openmp/runtime/unittests/CMakeLists.txt:17 (add_library):
Cannot find source file:
EXCLUDE_FROM_ALL
CMake Error at .../llvm-project/openmp/runtime/unittests/CMakeLists.txt:17 (add_library):
No SOURCES given to target: omp_testing
tho
| if(TARGET default_gtest) | ||
| target_link_libraries(${test_name} PRIVATE default_gtest_main default_gtest) | ||
| else () | ||
| target_link_libraries(${test_name} PRIVATE llvm_gtest_main llvm_gtest) | ||
| endif () |
There was a problem hiding this comment.
[not a change request] default_gtest exists to make this switch unnecessary. I assume you have it here for standalone builds?
There was a problem hiding this comment.
I used what you used in your PR you linked previously: https://github.com/llvm/llvm-project/pull/164794/changes#diff-f60dae8e82f67fa5e534e9ea566430d5b79f36119020865dc37b95d9a3635c5e
6 participants
(The tests in
TestKmpStr.cppare an automatically generated POC to make sure things work.)