Skip to content

[SYCL] Disable system OpenCL by default, pass the current cmake generator/command to build OpenCL #528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 6, 2019
Merged

[SYCL] Disable system OpenCL by default, pass the current cmake generator/command to build OpenCL #528

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 26 additions & 9 deletions sycl/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,20 @@ set(CLANG_VERSION "${CLANG_VERSION_MAJOR}.${CLANG_VERSION_MINOR}.${CLANG_VERSION
set(LLVM_INST_INC_DIRECTORY "lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/include")
set(dst_dir ${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION}/include)

find_package(OpenCL)
# Find OpenCL headers and libraries installed in the system and use them to
# build SYCL runtime.
# WARNING: use with caution, building SYCL runtime with OpenCL implementation
# instead of Khronos ICD loader might cause build and/or portability issues.
option(OpenCL_BUILD_WITH_SYSTEM_SDK OFF)

if( (OpenCL_INCLUDE_DIR AND OpenCL_LIBRARY) OR OpenCL_BUILD_WITH_SYSTEM_SDK)
find_package(OpenCL)
endif()

include(ExternalProject)

if( NOT OpenCL_INCLUDE_DIRS )
message ("OpenCL_INCLUDE_DIRS is missed. Try to download headers from github.com")
message("OpenCL_INCLUDE_DIRS is missed. Will try to download OpenCL headers from github.com")
set(OpenCL_INCLUDE_DIRS "${CMAKE_CURRENT_BINARY_DIR}/OpenCL/inc")
ExternalProject_Add(ocl-headers
GIT_REPOSITORY https://github.com/KhronosGroup/OpenCL-Headers.git
Expand All @@ -52,7 +60,6 @@ if( NOT OpenCL_INCLUDE_DIRS )
COMMENT "Downloading OpenCL headers."
)
else()
message("OpenCL header have been found under ${OpenCL_INCLUDE_DIRS}.")
add_custom_target( ocl-headers ALL
DEPENDS ${OpenCL_INCLUDE_DIRS}
COMMAND ${CMAKE_COMMAND} -E copy_directory ${OpenCL_INCLUDE_DIRS}/CL ${dst_dir}/CL
Expand All @@ -61,22 +68,32 @@ else()
endif()

if( NOT OpenCL_LIBRARIES )
message("OpenCL_LIBRARIES is missed. Try to build from GitHub sources.")
set(OpenCL_LIBRARIES "${LLVM_LIBRARY_OUTPUT_INTDIR}/libOpenCL.so")
message("OpenCL_LIBRARIES is missed. Will try to download OpenCL ICD Loader from github.com")
if(MSVC)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple notes.
There are dedicated CMake variables for library prefixes/suffixes:
https://gitlab.kitware.com/cmake/community/wikis/doc/cmake/Useful-Variables
CMAKE_ARGS accepts generator expressions. I believe it would be better way to set OPENCL_ICD_LOADER_REQUIRE_WDK that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valery, I suppose you mentioned the vars like CMAKE_STATIC_LIBRARY_SUFFIX.

This case is non-standard and those CMAKE vars will not work. CMAKE_STATIC_LIBRARY_SUFFIX does not work on Linux because it is lowered to ".a", while I need ".so".
CMAKE_SHARED_LIBRARY_SUFFIX does not work on Windows as it is lowered to ".dll", while I need ".lib".
Also, CMAKE_LINK_LIBRARY_SUFFIX does not work on Linux, it is simply empty there.

For some reasons using expr generator for OPENCL_ICD_LOADER_REQUIRE_WDK did not work for me. Also, in a separate e-mail thread you mentioned that you skip your requirement for using expr generator in this case.

Thus, this block of code remains the same. The only difference after your last review was the definition of OpenCL_VERSION_STRING. It was needed to avoid cmake errors when OpenCL_INCLUDE_DIR is not set AND it is not available in system.
Without that fix the expression:
"if( NOT OpenCL_INCLUDE_DIRS OR ${OpenCL_VERSION_STRING} VERSION_LESS 2.2)"
was lowered to this one with syntax error:
"if( NOT OpenCL_INCLUDE_DIRS OR VERSION_LESS 2.2)"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second @valerydmit here.
It looks like we are trying to link with OpenCL library differently depending on the platform.
IIRC, dynamic linking on Windows is slightly different from Linux, but it also should be handled by the cmake just fine. We just need communicate to cmake that "OpenCL is a library" and it shouldn't require low level manipulations with file extensions and compiler flags to link the library.

Copy link
Contributor Author

@v-klochkov v-klochkov Sep 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some reasons why find_package(OpenCL) is needed when those 2 vars are set:

  1. If I remove '(OpenCL_INCLUDE_DIR AND OpenCL_LIBRARY) OR ' then OpenCL is NOT FOUND
    even if I manually set -DOpenCL_INCLUDE_DIR=$OPENCL_HEADERS -DOpenCL_LIBRARY=$OPENCL_LOADER

    OpenCL_INCLUDE_DIR is missed. Will try to download OpenCL headers from github.com
    OpenCL_LIBRARY is missed. Will try to download OpenCL ICD Loader from github.com

    That happens because we set OpenCL_INCLUDE_DIR and then check OpenCL_INCLUDE_DIRS below (DIR vs DIRS). find_package(OpenCL) sets OpenCL_INCLUDE_DIRS.

  2. find_package(OpenCL) initializes some other useful vars like OPENCL_FOUND, etc.

Regarding your concern "their values might be reset(!) by this function".
I tried that. It does NOT re-define what you explicitly defined. OpenCL_INCLUDE_DIR has the highest priority:
a) "-DOpenCL_INCLUDE_DIR=garbage -DOpenCL_LIBRARY=garbage" ==> OpenCL is FOUND, not even checked.
-- Looking for CL_VERSION_2_2
-- Looking for CL_VERSION_2_2 - not found
-- Looking for CL_VERSION_2_1
-- Looking for CL_VERSION_2_1 - not found
-- Looking for CL_VERSION_2_0
-- Looking for CL_VERSION_2_0 - not found
-- Looking for CL_VERSION_1_2
-- Looking for CL_VERSION_1_2 - not found
-- Looking for CL_VERSION_1_1
-- Looking for CL_VERSION_1_1 - not found
-- Looking for CL_VERSION_1_0
-- Looking for CL_VERSION_1_0 - not found
-- Found OpenCL: D:/iusers//ws/gs_public/build_ninja/garbage

b) "-DBUILD_WITH_SYSTEM_OPENCL=ON -DOpenCL_INCLUDE_DIR=garbage -DOpenCL_LIBRARY=garbage" ==> SAME OUTPUT AS IN (a) above).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Should we just unify naming across the whole project?
  2. Do you have OpenCL installed on you system? I.e. what will happen if these variables are not set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Should we just unify naming across the whole project?

I used the name 'BUILD_WITH_SYSTEM_OPENCL' suggested by you. Should I change it to OpenCL_BUILD_WITH_SYSTEM_SDK ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2. Do you have OpenCL installed on you system? I.e. what will happen if these variables are not set?

If OpenCL_INCLUDE_DIR and OpenCL_LIBRARY are not set, then

a) if BUILD_WITH_SYSTEM_OPENCL unset, then nothing is found, and they are downloaded/built from github.

b) if BUILD_WITH_SYSTEM_OPENCL is set then SYSTEM SDK may be found. I have OpenCL headers 2.1 on my lab machine, it is found and tried to be used.
BTW, currently SYCL compiler cannot be built with 2.1 headers because we include both files cl_ext.h and cl_ext_intel.h and they define same structures in 2.1, but not in 2.2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Should we just unify naming across the whole project?

I used the name 'BUILD_WITH_SYSTEM_OPENCL' suggested by you. Should I change it to OpenCL_BUILD_WITH_SYSTEM_SDK ?

No.

Sorry I wasn't clear enough. I was talking about OpenCL_INCLUDE_DIR/OpenCL_INCLUDE_DIRS and OpenCL_LIBRARY/DOpenCL_LIBRARIES.

Anyway. Let's fix more important issues first and refactor it later.

set(OpenCL_LIBRARIES
"${LLVM_LIBRARY_OUTPUT_INTDIR}/${CMAKE_STATIC_LIBRARY_PREFIX}OpenCL${CMAKE_STATIC_LIBRARY_SUFFIX}")
set(OPENCL_ICD_LOADER_WDK "-DOPENCL_ICD_LOADER_REQUIRE_WDK=OFF")
else()
set(OpenCL_LIBRARIES
"${LLVM_LIBRARY_OUTPUT_INTDIR}/${CMAKE_SHARED_LIBRARY_PREFIX}OpenCL${CMAKE_SHARED_LIBRARY_SUFFIX}")
set(OPENCL_ICD_LOADER_WDK "")
endif()
file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/icd_build)
ExternalProject_Add(ocl-icd
GIT_REPOSITORY https://github.com/KhronosGroup/OpenCL-ICD-Loader.git
GIT_TAG origin/master
SOURCE_DIR "${CMAKE_CURRENT_BINARY_DIR}/OpenCL/icd"
BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/icd_build"
CONFIGURE_COMMAND ${CMAKE_COMMAND} "${CMAKE_CURRENT_BINARY_DIR}/OpenCL/icd" -DCMAKE_INSTALL_LIBDIR:PATH=lib -DCMAKE_INSTALL_PREFIX=${LLVM_BINARY_DIR}
BUILD_COMMAND make C_INCLUDE_PATH=${CMAKE_CURRENT_BINARY_DIR}/OpenCL/inc
INSTALL_COMMAND make install
CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
-DCMAKE_MAKE_PROGRAM=${CMAKE_MAKE_PROGRAM}
-DOPENCL_ICD_LOADER_HEADERS_DIR=${CMAKE_CURRENT_BINARY_DIR}/OpenCL/inc
-DCMAKE_INSTALL_PREFIX=${LLVM_BINARY_DIR}
-DCMAKE_INSTALL_LIBDIR:PATH=lib
${OPENCL_ICD_LOADER_WDK}
STEP_TARGETS configure,build,install
DEPENDS ocl-headers
)
else()
message("OpenCL loader has been found: ${OpenCL_LIBRARIES}.")
file(GLOB ICD_LOADER_SRC "${OpenCL_LIBRARIES}*")
file(COPY ${ICD_LOADER_SRC} DESTINATION ${LLVM_LIBRARY_OUTPUT_INTDIR})
add_custom_target(ocl-icd DEPENDS ${OpenCL_LIBRARIES} COMMENT "Copying OpenCL ICD Loader ...")
Expand Down