Add optional add_sycl_to_target to cmake #2
Conversation
CMakeLists.txt
Outdated
| find_package(Compiler REQUIRED) | ||
| if (ONEMKL_SYCL_IMPLEMENTATION STREQUAL "hipSYCL") | ||
| message(STATUS "Looking for hipSYCL") | ||
| find_package(hipSYCL CONFIG PATHS /opt/hipSYCL/lib/cmake/ REQUIRED) |
There was a problem hiding this comment.
I think we can remove the PATHS specification to make it work better with non-system-wide installations. (In any case, I think it should be lib/cmake/hipSYCL, not lib/cmake)
There was a problem hiding this comment.
I think having PATHS is nice since it makes oneMKL just work out of the box with a default installation of hipSYCL, but I understand that this might get confusing if the user has multiple hipSYCL installations available. Hower in case of a non-default installation hipSYCL_ROOT, or hipSYCL_DIR has to be specified anyways right?
(indeed the path seems to be wrong thanks!)
sbalint98
force-pushed
the
test-port-cmake
branch
3 times, most recently
from
ed2d6c3 to
835888a
Compare
|
removed PATHS and rebased to current master of oneMKL, still have to run the tests somehow |
sbalint98
force-pushed
the
test-port-cmake
branch
from
6537bdf to
5f0ff23
Compare
sbalint98
force-pushed
the
test-port-cmake
branch
from
5f0ff23 to
f51ff59
Compare
| @@ -68,6 +68,10 @@ endif() | |||
| option(ENABLE_CUBLAS_BACKEND "" OFF) | |||
| option(ENABLE_CURAND_BACKEND "" OFF) | |||
| option(ENABLE_NETLIB_BACKEND "" OFF) | |||
| option(DISABLE_HALF_RUTINES "" OFF) | |||
There was a problem hiding this comment.
Ah here it is... an alternative naming could be ENABLE_HALF_DATA_TYPE or similar (inspired by SYCL-BLAS..)
CMakeLists.txt
Outdated
| @@ -124,7 +128,20 @@ list(APPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake") | |||
| if(WIN32) | |||
| add_library(ONEMKL::SYCL::SYCL INTERFACE IMPORTED) | |||
| else() | |||
| find_package(Compiler REQUIRED) | |||
| # Find necessary packages | |||
| if (${ONEMKL_SYCL_IMPLEMENTATION} STREQUAL "hipSYCL") | |||
There was a problem hiding this comment.
I think I'd prefer to use hipsycl to be in-line with using lowercase dpc++..
you could also use string( TOLOWER "${ONEMKL_SYCL_IMPLEMENTATION}" ONEMKL_SYCL_IMPLEMENTATION) beforehand, so the case of the input actually doesn't matter.
Also, if I understand it correctly, it is better to use if(ONEMKL_SYCL_IMPLEMENTATION .. without the dereferencing source
There was a problem hiding this comment.
I think I'm inclined to disagree about hipSYCL being lowercase. SYCL is a term that is inherently and strictly defined as all capital. DPC++ does not have such strong capitalization definitions. We also pretty consistently use the hipSYCL capitalization in all our stuff - the only exception being C++ source code inside hipSYCL.
In any case, actively turning everything lower case and hence accepting any capitalization seems like the best way to go.
| @@ -42,7 +41,9 @@ set_target_properties(${LIB_OBJ} PROPERTIES | |||
| POSITION_INDEPENDENT_CODE ON) | |||
|
|
|||
| target_link_libraries(${LIB_NAME} PUBLIC ${LIB_OBJ}) | |||
|
|
|||
| if (USE_ADD_SYCL_TO_TARGET_INTEGRATION) | |||
| add_sycl_to_target(TARGET ${LIB_OBJ} SOURCES ${SOURCES}) | |||
There was a problem hiding this comment.
do we even have to specify the SOURCES here..? (not sure about the semantics with or without :D)
There was a problem hiding this comment.
hipSYCL doesn't need it, but it's the correct interface for portability, as e.g. ComputeCpp needs it AFAIK.
| add_library(${LIB_NAME}) | ||
| add_library(${LIB_OBJ} OBJECT ${SOURCES}) | ||
| if (USE_ADD_SYCL_TO_TARGET_INTEGRATION) | ||
| add_sycl_to_target(TARGET ${LIB_OBJ} ${SOURCES}) |
There was a problem hiding this comment.
if you specify the sources here, don't you need the SOURCES keyword, as you used above?
tests/unit_tests/CMakeLists.txt
Outdated
| @@ -59,6 +59,9 @@ foreach(domain ${TARGET_DOMAINS}) | |||
| add_executable(test_main_${domain}_ct main_test.cpp) | |||
| target_include_directories(test_main_${domain}_ct PUBLIC ${GTEST_INCLUDE_DIR}) | |||
| target_compile_options(test_main_${domain}_ct PRIVATE -fsycl) | |||
| if (USE_ADD_SYCL_TO_TARGET_INTEGRATION) | |||
| add_sycl_to_target(TARGET test_main_${domain}_ct SOURCES t main_test.cpp) | |||
There was a problem hiding this comment.
t seems like a good source file name..? :D
also, shouldn't the setting of the -fsycl flag be moved to the else branch (potentially even only if the implementation option is set to dpc++)?
| add_sycl_to_target(TARGET blas_batch_ct SOURCES ${BATCH_SOURCES}) | ||
| else() | ||
| target_link_libraries(blas_batch_ct PUBLIC ONEMKL::SYCL::SYCL) | ||
| endif() |
Description
Added
add_sycl_to_targetcmake integration. This makes it more straightforward to compile oneMKL with different SYCL implementations.Motivation
These changes allow other SYCL implementations to compile and use oneMKL
All Submissions
Do all unit tests pass locally? Attach a log.
ct_cpu_test.log
rt_cpu_tests.log
Have you formatted the code using clang-format?Have you provided motivation for adding a new feature?Have you added relevant tests?