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

v-klochkov · 2019-08-19T15:23:14Z

…indows

Signed-off-by: Vyacheslav N Klochkov [email protected]

sycl/CMakeLists.txt

valerydmit · 2019-08-22T21:17:58Z

sycl/CMakeLists.txt

-  set(OpenCL_LIBRARIES "${LLVM_LIBRARY_OUTPUT_INTDIR}/libOpenCL.so")
+if( NOT OpenCL_LIBRARIES OR ${OpenCL_VERSION_STRING} VERSION_LESS 2.2 )
+  message("OpenCL_LIBRARY is missed or disregarded. Will try to download OpenCL ICD Loader 2.2 or newer from github.com")
+  if(MSVC)


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.

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)"

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.

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

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.

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).

Should we just unify naming across the whole project?

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

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 ?

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.

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.

sycl/CMakeLists.txt

valerydmit

Looks good to me.

bader · 2019-09-05T17:08:11Z

sycl/CMakeLists.txt

+# instead of Khronos ICD loader might cause build and/or portability issues.
+option(BUILD_WITH_SYSTEM_OPENCL OFF)
+
+if( (OpenCL_INCLUDE_DIR AND OpenCL_LIBRARY) OR BUILD_WITH_SYSTEM_OPENCL)


Probably setting both BUILD_WITH_SYSTEM_OPENCL and OpenCL_* variables at the same time should give at least a warning to let the user know which option has higher priority.

Interesting idea, but are you sure that warning is really needed?
I mean if some users really reads those messages from configure step, then that user will see what version was chosen. find_package(OpenCL) explicitly writes that. For example, on a system with OpenCL available in the system and cmake flags:

-DOpenCL_INCLUDE_DIR=C:/Users/vklochko/ws/gs_public/OpenCL-Headers -DOpenCL_LIBRARY=C:/Users/vklochko/ws/gs_public/OpenCL-ICD-Loader/build/OpenCL.lib -DOpenCL_BUILD_WITH_SYSTEM_SDK=ON

Then cmake writes:
-- Found OpenCL: C:/Users/vklochko/ws/gs_public/OpenCL-ICD-Loader/build/OpenCL.lib (found version "2.2")

This message makes it pretty obvious that those 2 vars def have higher priority than system sdk.
Do you agree?

This patch 1) Disables using system OpenCL Loader/Header by default. 2) Re-uses the currently used cmake generator/command to build OpenCL Loader. 3) Enables automatic download and build of OpenCL.lib on Windows. Signed-off-by: Vyacheslav N Klochkov <[email protected]>

v-klochkov · 2019-09-05T20:17:47Z

The new patch update changed only the option name:
BUILD_WITH_SYSTEM_OPENCL -> OpenCL_BUILD_WITH_SYSTEM_SDK

v-klochkov requested a review from bader August 19, 2019 15:23

v-klochkov mentioned this pull request Aug 19, 2019

[SYCL][DOC] Add build instructions for Windows to GetStartedWithSYCLC… #494

Merged

bader reviewed Aug 19, 2019

View reviewed changes

sycl/CMakeLists.txt Outdated Show resolved Hide resolved

v-klochkov requested review from valerydmit and bader August 21, 2019 20:03

valerydmit reviewed Aug 22, 2019

View reviewed changes

v-klochkov force-pushed the public_OpenCL_dirs branch 2 times, most recently from 304107e to 68fed25 Compare August 27, 2019 06:22

v-klochkov requested a review from valerydmit August 27, 2019 06:30

v-klochkov force-pushed the public_OpenCL_dirs branch from 68fed25 to 3bc58e5 Compare September 3, 2019 16:50

v-klochkov changed the title ~~[SYCL] Enable lazy OpenCL ICD Loader and Headers download/build for W…~~ [SYCL] Disable system OpenCL by default, pass the current cmake generator/command to build OpenCL Sep 3, 2019

bader reviewed Sep 3, 2019

View reviewed changes

sycl/CMakeLists.txt Outdated Show resolved Hide resolved

valerydmit reviewed Sep 3, 2019

View reviewed changes

sycl/CMakeLists.txt Outdated Show resolved Hide resolved

v-klochkov force-pushed the public_OpenCL_dirs branch 2 times, most recently from 6a51aba to 02f20f1 Compare September 4, 2019 15:28

valerydmit previously approved these changes Sep 4, 2019

View reviewed changes

v-klochkov requested a review from bader September 4, 2019 17:16

bader reviewed Sep 5, 2019

View reviewed changes

v-klochkov dismissed valerydmit’s stale review via 9b9f473 September 5, 2019 20:16

v-klochkov force-pushed the public_OpenCL_dirs branch from 02f20f1 to 9b9f473 Compare September 5, 2019 20:16

v-klochkov requested a review from bader September 5, 2019 20:19

v-klochkov mentioned this pull request Sep 5, 2019

[Windows] CMakeLists.txt requires fixing/improvements #605

Closed

bader approved these changes Sep 6, 2019

View reviewed changes

bader merged commit 590e79b into intel:sycl Sep 6, 2019

v-klochkov deleted the public_OpenCL_dirs branch September 10, 2019 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

v-klochkov commented Aug 19, 2019

Uh oh!

Uh oh!

valerydmit Aug 22, 2019

Uh oh!

v-klochkov Aug 27, 2019

Uh oh!

bader Aug 27, 2019

Uh oh!

v-klochkov Sep 3, 2019 •

edited

Loading

Uh oh!

bader Sep 5, 2019

Uh oh!

v-klochkov Sep 5, 2019

Uh oh!

v-klochkov Sep 5, 2019

Uh oh!

bader Sep 6, 2019

Uh oh!

Uh oh!

Uh oh!

valerydmit left a comment

Uh oh!

bader Sep 5, 2019

Uh oh!

v-klochkov Sep 5, 2019

Uh oh!

v-klochkov commented Sep 5, 2019

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

v-klochkov commented Aug 19, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v-klochkov Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

valerydmit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v-klochkov commented Sep 5, 2019

Uh oh!

Uh oh!

v-klochkov Sep 3, 2019 •

edited

Loading