[SYCL][Cmake] Redesign OpenCL dependencies #3485

pvchupin · 2021-04-04T07:49:30Z

Moved from ExternalProject to FetchContent:
- External dependencies are downloaded at configure stage, not build
- All dependencies targets become available earlier and become reusable
- All dependencies are consolidated under _deps directory
- Simplified cmake files for better maintenance
Outline opencl deps into separate opencl/CMakeLists.txt
Update other targets to new deps accordingly
Cleaned up CUDA plugin and CUDA unittest target dependencies
Removed broken --system-ocl configure feature
Added comments inlined for not very clear logic in our cmake build system

Signed-off-by: Pavel V Chupin pavel.v.chupin@intel.com

sycl/plugins/opencl/CMakeLists.txt

smaslov-intel

LGTM

* Moved from ExternalProject to FetchContent: * External dependencies are downloaded at configure stage, not build * All dependencies targets become available earlier and become reusable * All dependencies are consolidated under _deps directory * Simplified cmake files for better maintenance * Outline opencl deps into separate opencl/CMakeLists.txt * Update other targets to new deps accordingly * Added comments inlined for not very clear logic in our cmake build system Signed-off-by: Pavel V Chupin <pavel.v.chupin@intel.com>

pvchupin · 2021-04-14T15:50:04Z

Finished the cleanup. Now it's ready for review.
I'm planning to do separate follow up patches. Next one is switching opencl-aot to reuse the same OpenCL deps.
I'm also thinking on moving opencl-aot into opencl folder. Let me know what you think about idea and potential impact.

sycl/CMakeLists.txt

opencl/CMakeLists.txt

sycl/CMakeLists.txt

dm-vodopyanov · 2021-04-14T20:42:01Z

I'm also thinking on moving opencl-aot into opencl folder. Let me know what you think about idea and potential impact.

LGTM to move opencl-aot to opencl folder. The only problem is that opencl-aot history will be lost. It looks like there is a way to see the history before the movement, but it is unideal and may not work correctly sometimes. Anyway, there are 3-4 contributors to this component and it is ok to lose the history.

bader

Thanks for cleaning this up.
opencl/CMakeLists.txt looks good to me with one minor comment.

opencl/CMakeLists.txt

bader · 2021-04-15T17:10:48Z

@pvchupin, please, resolve merge conflicts.

alexbatashev

LGTM

opencl/CMakeLists.txt

sycl/plugins/cuda/CMakeLists.txt

pvchupin

Thank you all for good review. I'm going to try suggestions mentioned.
Give me some time for the next iteration.

opencl/CMakeLists.txt

sycl/plugins/cuda/CMakeLists.txt

pvchupin · 2021-04-27T06:38:31Z

-system-ocl configure?

It didn't work for me. @vladimirlaz suggested a few tricks with paths, but it didn't help. Either it has some special undocumented prerequisites, or it's just broken for a while. We decided to cleanup the vars that's been used and I went further in removing the option from the script. These deps are downloaded in a few seconds anyway with the specific version we want.

bader · 2021-04-27T06:38:41Z

@pvchupin, FYI. BuildBot CI explicitly builds icd-loader in a separate dependency step: http://ci.llvm.intel.com:8010/#/builders/2/builds/10563/steps/7/logs/stdio.

I think this patch will introduce unnecessary redundancy in building ICD loader twice.
IMO, we should have an option to skip fetching and building OpenCL ICD-loader.

bader · 2021-04-27T06:39:37Z

-system-ocl configure?

It didn't work for me. @vladimirlaz suggested a few tricks with paths, but it didn't help. Either it has some special undocumented prerequisites, or it's just broken for a while. We decided to cleanup the vars that's been used and I went further in removing the option from the script. These deps are downloaded in a few seconds anyway with the specific version we want.

I think we should fix this feature rather than removing it.

pvchupin · 2021-04-27T06:41:09Z

@pvchupin, FYI. BuildBot CI explicitly builds icd-loader in a separate dependency step: http://ci.llvm.intel.com:8010/#/builders/2/builds/10563/steps/7/logs/stdio.

I think this patch will introduce unnecessary redundancy in building ICD loader twice.
IMO, we should have an option to skip fetching and building OpenCL ICD-loader.

Currently it's built twice because opencl-aot builds it separately. Note that it's been built twice before my patch. Next patch I'm going to make for opencl-aot will start reusing dependency.

pvchupin · 2021-04-27T06:44:12Z

I think we should fix this feature rather than removing it.

Can you or anybody specify what this feature was supposed to do and when/how it was working when it was working? Or how to use it now if it's still working for anybody.
I don't see much value to fix it if it's broken indeed.

pvchupin · 2021-04-27T06:48:58Z

@pvchupin, FYI. BuildBot CI explicitly builds icd-loader in a separate dependency step:

Thanks. I didn't know that. Will take a closer look tomorrow. Probably we should remove the whole dependency step. We have more dependencies right now than just opencl (level_zero, vc_intrinsics) and this script is getting opencl only. Can be fetched/reused at configure stage instead.

sycl/unittests/pi/cuda/CMakeLists.txt

sycl/plugins/opencl/CMakeLists.txt

sycl/plugins/level_zero/CMakeLists.txt

bader · 2021-04-27T06:55:11Z

I think we should fix this feature rather than removing it.

Can you or anybody specify what this feature was supposed to do and when/how it was working when it was working? Or how to use it now if it's still working for anybody.
I don't see much value to fix it if it's broken indeed.

I use it and works for me.
I manually fetched OpenCL headers and OpenCL-ICD loader sources locally, built OpenCL-ICD loader library and set their location using cmake variables for LLVM build configuration. I expect that when I build SYCL project, fetching and building OpenCL headers and ICD loader library is skipped.

bader · 2021-04-27T06:57:41Z

I think we should fix this feature rather than removing it.

Can you or anybody specify what this feature was supposed to do and when/how it was working when it was working? Or how to use it now if it's still working for anybody.
I don't see much value to fix it if it's broken indeed.

I use it and works for me.
I manually fetched OpenCL headers and OpenCL-ICD loader sources locally, built OpenCL-ICD loader library and set their location using cmake variables for LLVM build configuration. I expect that when I build SYCL project, fetching and building OpenCL headers and ICD loader library is skipped.

This feature is critical for scenarios when we need to build SYCL project on the system w/o access to the Internet/GitHub.

sycl/plugins/opencl/CMakeLists.txt

Co-authored-by: Alexey Bader <alexey.bader@intel.com> Co-authored-by: smaslov-intel <48694368+smaslov-intel@users.noreply.github.com>

Can be used by configure.py --cmake-opt="-DOpenCL_HEADERS=path" --cmake-opt="-DOpenCL_LIBRARY_SRC=path"

pvchupin · 2021-04-27T23:08:32Z

@bader as discussed added possibility to use predownloaded headers and library sources through cmake flags.

bader

@vladimirlaz, please, take a look.

vladimirlaz

LGTM
Some ideas next improvements (in next PR):

apply the same changes for opencl-aot;
remove dependency.py script as all its artifacts are ignored;
add CMake variable for storing GIT_TAG for both OpenCL headers and ICD loader (like we have in https://github.com/intel/llvm/blob/sycl/llvm/lib/SYCLLowerIR/CMakeLists.txt#L7)

pvchupin · 2021-04-30T07:13:03Z

Adding one last change in this PR, actually the last from @vladimirlaz todo list. It will allow easier customizations of these if needed.
Other TODOs will be in separate PRs.

pvchupin requested review from bader, smaslov-intel and a team as code owners April 4, 2021 07:49

pvchupin marked this pull request as draft April 4, 2021 07:52

pvchupin force-pushed the opencl-cmake1 branch 3 times, most recently from 47665e9 to 40af3a5 Compare April 5, 2021 05:14

smaslov-intel reviewed Apr 5, 2021

View reviewed changes

sycl/plugins/opencl/CMakeLists.txt Outdated Show resolved Hide resolved

smaslov-intel previously approved these changes Apr 5, 2021

View reviewed changes

pvchupin dismissed smaslov-intel’s stale review via 2f8426c April 12, 2021 06:23

pvchupin force-pushed the opencl-cmake1 branch 2 times, most recently from 2f0171b to 5300a98 Compare April 13, 2021 04:55

pvchupin force-pushed the opencl-cmake1 branch from 5300a98 to eb1d87a Compare April 14, 2021 07:24

pvchupin marked this pull request as ready for review April 14, 2021 15:47

pvchupin requested a review from vladimirlaz April 14, 2021 15:48

pvchupin requested a review from dm-vodopyanov April 14, 2021 15:50

vladimirlaz suggested changes Apr 14, 2021

View reviewed changes

bader requested a review from alexbatashev April 15, 2021 16:42

bader previously approved these changes Apr 15, 2021

View reviewed changes

opencl/CMakeLists.txt Show resolved Hide resolved

alexbatashev previously approved these changes Apr 15, 2021

View reviewed changes

opencl/CMakeLists.txt Show resolved Hide resolved

sycl/plugins/cuda/CMakeLists.txt Outdated Show resolved Hide resolved

pvchupin commented Apr 15, 2021

View reviewed changes

opencl/CMakeLists.txt Show resolved Hide resolved

opencl/CMakeLists.txt Outdated Show resolved Hide resolved

sycl/plugins/cuda/CMakeLists.txt Outdated Show resolved Hide resolved

Merge branch 'sycl' into opencl-cmake1

adf882f

pvchupin dismissed stale reviews from alexbatashev and bader via adf882f April 16, 2021 05:04

Moving OpenCL shared libs enforcement to OpenCL CMakeLists.txt

66392bb

bader changed the title ~~[SYCL][Cmake] Redesign opencl dependencies~~ [SYCL][Cmake] Redesign OpenCL dependencies Apr 27, 2021

bader reviewed Apr 27, 2021

View reviewed changes

sycl/unittests/pi/cuda/CMakeLists.txt Outdated Show resolved Hide resolved

sycl/plugins/opencl/CMakeLists.txt Outdated Show resolved Hide resolved

sycl/plugins/level_zero/CMakeLists.txt Outdated Show resolved Hide resolved

smaslov-intel reviewed Apr 27, 2021

View reviewed changes

sycl/plugins/opencl/CMakeLists.txt Outdated Show resolved Hide resolved

smaslov-intel reviewed Apr 27, 2021

View reviewed changes

sycl/plugins/opencl/CMakeLists.txt Outdated Show resolved Hide resolved

smaslov-intel previously approved these changes Apr 27, 2021

View reviewed changes

Apply suggestions from code review

f3779c1

Co-authored-by: Alexey Bader <alexey.bader@intel.com> Co-authored-by: smaslov-intel <48694368+smaslov-intel@users.noreply.github.com>

pvchupin dismissed smaslov-intel’s stale review via f3779c1 April 27, 2021 20:36

Add feature to use pre-downloaded OpenCL deps sources

71d4b0f

Can be used by configure.py --cmake-opt="-DOpenCL_HEADERS=path" --cmake-opt="-DOpenCL_LIBRARY_SRC=path"

bader previously approved these changes Apr 28, 2021

View reviewed changes

vladimirlaz previously approved these changes Apr 28, 2021

View reviewed changes

bader requested a review from smaslov-intel April 29, 2021 19:34

Outline URLs and repo tags/hashes definitions

9dd80bd

pvchupin dismissed stale reviews from vladimirlaz and bader via 9dd80bd April 30, 2021 07:10

bader requested a review from vladimirlaz April 30, 2021 07:19

bader approved these changes Apr 30, 2021

View reviewed changes

vladimirlaz approved these changes Apr 30, 2021

View reviewed changes

smaslov-intel approved these changes Apr 30, 2021

View reviewed changes

pvchupin merged commit 66919a2 into intel:sycl Apr 30, 2021

pvchupin deleted the opencl-cmake1 branch April 30, 2021 17:36

[SYCL][Cmake] Redesign OpenCL dependencies #3485

[SYCL][Cmake] Redesign OpenCL dependencies #3485

Uh oh!

Conversation

pvchupin commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

pvchupin commented Apr 14, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dm-vodopyanov commented Apr 14, 2021

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bader commented Apr 15, 2021

Uh oh!

alexbatashev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pvchupin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pvchupin commented Apr 27, 2021

Uh oh!

bader commented Apr 27, 2021

Uh oh!

bader commented Apr 27, 2021

Uh oh!

pvchupin commented Apr 27, 2021

Uh oh!

pvchupin commented Apr 27, 2021

Uh oh!

pvchupin commented Apr 27, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bader commented Apr 27, 2021

Uh oh!

bader commented Apr 27, 2021

Uh oh!

Uh oh!

Uh oh!

pvchupin commented Apr 27, 2021

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

vladimirlaz left a comment

Choose a reason for hiding this comment

Uh oh!

pvchupin commented Apr 30, 2021

Uh oh!

Uh oh!

pvchupin commented Apr 4, 2021 •

edited

Loading