[SYCL][ESIMD] Bring vc-intrinsics sources #1930

asi-sc · 2020-06-19T12:52:42Z

VC intrinsics library is required by LowerESIMD pass (#1881).

sycl/CMakeLists.txt

v-klochkov · 2020-06-19T17:02:23Z

sycl/CMakeLists.txt

@@ -184,6 +184,21 @@ install(DIRECTORY ${OPENCL_INCLUDE}/CL
  COMPONENT opencl-headers
 )

+if (NOT TARGET LLVMGenXIntrinsics)
+  message(STATUS "vc-intrinsics are missing. Will try to download them from github.com")
+  include(FetchContent)


The line 1 in this CmakeLists.txt file says:
cmake_minimum_required(VERSION 3.2)
But the FetchContent becomes available only in Cmake-3.11

Is there any specific reason why you need FetchContent and move the download to 'configure' phase instead of using ExternalProject and have the download at 'build' phase?

We have a long discussion in an email tread. The reason for this is a cyclic dependency LLVM->vc-instrinsics->LLVM (LowerESIMD). And we decided that I can use this mechanism,

Please add those people to reviewers.
If you keep using this mechanism, then please a) update the first line in this file and b) update the pre-requisites here: https://intel.github.io/llvm-docs/GetStartedGuide.html#prerequisites

OK, done. I set 3.14 version because FetchContent_MakeAvailable was introduced at this version.

vladimirlaz

approve to start testin

sycl/CMakeLists.txt

vladimirlaz

please add possibility to provide source directory in cmake command line.

VC intrinsics library is required by LowerESIMD pass.

v-klochkov

The changes look good to me.
Raising the cmake version from 3.2 to 3.14 may need approval from @bader and/or @pvchupin

kbobrovs

Doc change LGTM

kbobrovs · 2020-06-25T23:05:13Z

@bader - please take a look, leave it to you to merge.

bader · 2020-06-26T13:58:50Z

VC intrinsics library is required by LowerESIMD pass (#1881).

It seems to me that it would be more reasonable to move CMake changes from SYCL library CMake to llvm/lib/SYCLLowerIR/CMakeLists.txt.
I think it would be much easier to manage SYCLLowerIR pass with it's dependencies.
@asidoren-intel, don't you think so?
I you agree, I suggest moving this patch to PR #1881.

bader · 2020-06-26T14:04:57Z

The changes look good to me.
Raising the cmake version from 3.2 to 3.14 may need approval from @bader and/or @pvchupin

@tfzhu, @vladimirlaz, is CI environment ready for such change?

vladimirlaz · 2020-06-26T14:13:35Z

The changes look good to me.
Raising the cmake version from 3.2 to 3.14 may need approval from @bader and/or @pvchupin

@tfzhu, @vladimirlaz, is CI environment ready for such change?

We already moved all CIs (to 3.15 to match future LLVM expectations)

v-klochkov · 2020-06-26T18:32:19Z

It seems to me that it would be more reasonable to move CMake changes from SYCL library CMake to llvm/lib/SYCLLowerIR/CMakeLists.txt.
I think it would be much easier to manage SYCLLowerIR pass with it's dependencies.
@asidoren-intel, don't you think so?
I you agree, I suggest moving this patch to PR #1881.

Ops, sorry, I did not notice this comment when clicked Merge button.

bader · 2020-06-26T18:57:02Z

It seems to me that it would be more reasonable to move CMake changes from SYCL library CMake to llvm/lib/SYCLLowerIR/CMakeLists.txt.
I think it would be much easier to manage SYCLLowerIR pass with it's dependencies.
@asidoren-intel, don't you think so?
I you agree, I suggest moving this patch to PR #1881.

Ops, sorry, I did not notice this comment when clicked Merge button.

I think it can be applied in scope of #1881 i.e. moving CMake changes from SYCL library to LLVM pass.

asi-sc · 2020-06-29T11:28:37Z

It seems to me that it would be more reasonable to move CMake changes from SYCL library CMake to llvm/lib/SYCLLowerIR/CMakeLists.txt.
I think it would be much easier to manage SYCLLowerIR pass with it's dependencies.
@asidoren-intel, don't you think so?
I you agree, I suggest moving this patch to PR #1881.

Ops, sorry, I did not notice this comment when clicked Merge button.

I think it can be applied in scope of #1881 i.e. moving CMake changes from SYCL library to LLVM pass.

As for me, it depends on the further usage of vc-intrinsics. For now, this library is required only in SYCLLowerIR and it may be reasonably moved there. Can we imagine another user of vc-intrinsics library here? If we can, I'm not sure that we should move it.

bader · 2020-06-29T11:37:14Z

There are no usages of this intrinsics in the SYCL runtime library, so I don't understand why it's added to sycl/CMakeLists.txt in the first place.

kbobrovs · 2020-06-29T18:16:19Z

@asidoren-intel, I think such need in vc-intrinsics is quite unlikely for now, so I agree with @bader 's comment - will update #1881 and move this to SYCLLowerIR to get better encapsulation.

asi-sc requested a review from a team as a code owner June 19, 2020 12:52

asi-sc requested a review from v-klochkov June 19, 2020 12:52

v-klochkov reviewed Jun 19, 2020

View reviewed changes

sycl/CMakeLists.txt Outdated Show resolved Hide resolved

v-klochkov reviewed Jun 19, 2020

View reviewed changes

v-klochkov requested review from a team and bader June 19, 2020 17:03

kbobrovs requested a review from vladimirlaz June 19, 2020 18:45

vladimirlaz approved these changes Jun 21, 2020

View reviewed changes

sycl/CMakeLists.txt Show resolved Hide resolved

vladimirlaz self-requested a review June 21, 2020 14:52

vladimirlaz suggested changes Jun 21, 2020

View reviewed changes

[SYCL][ESIMD] Bring vc-intrinsics sources

4cb6975

VC intrinsics library is required by LowerESIMD pass.

asi-sc force-pushed the sycl branch from ee42f37 to 4cb6975 Compare June 22, 2020 13:08

asi-sc requested a review from pvchupin as a code owner June 22, 2020 13:08

vladimirlaz approved these changes Jun 22, 2020

View reviewed changes

v-klochkov approved these changes Jun 22, 2020

View reviewed changes

kbobrovs approved these changes Jun 25, 2020

View reviewed changes

kbobrovs added the esimd Explicit SIMD feature label Jun 25, 2020

v-klochkov merged commit afd7f4a into intel:sycl Jun 26, 2020

[SYCL][ESIMD] Bring vc-intrinsics sources #1930

[SYCL][ESIMD] Bring vc-intrinsics sources #1930

Uh oh!

Conversation

asi-sc commented Jun 19, 2020

Uh oh!

Uh oh!

v-klochkov Jun 19, 2020 • 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!

asi-sc Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

v-klochkov Jun 19, 2020 • 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!

asi-sc Jun 22, 2020

Choose a reason for hiding this comment

Uh oh!

vladimirlaz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vladimirlaz left a comment

Choose a reason for hiding this comment

Uh oh!

v-klochkov left a comment • 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!

kbobrovs left a comment

Choose a reason for hiding this comment

Uh oh!

kbobrovs commented Jun 25, 2020

Uh oh!

bader commented Jun 26, 2020

Uh oh!

bader commented Jun 26, 2020

Uh oh!

vladimirlaz commented Jun 26, 2020

Uh oh!

v-klochkov commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bader commented Jun 26, 2020

Uh oh!

asi-sc commented Jun 29, 2020

Uh oh!

bader commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbobrovs commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

v-klochkov Jun 19, 2020 •

edited

Loading

v-klochkov Jun 19, 2020 •

edited

Loading

v-klochkov left a comment •

edited

Loading

v-klochkov commented Jun 26, 2020 •

edited

Loading

bader commented Jun 29, 2020 •

edited

Loading

kbobrovs commented Jun 29, 2020 •

edited

Loading