Skip to content

[SYCL][AOT] Define CL_TARGET_OPENCL_VERSION to OpenCL 2.2 #1608

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

Jump to bottom
Merged
merged 3 commits into from
May 7, 2020
Merged

[SYCL][AOT] Define CL_TARGET_OPENCL_VERSION to OpenCL 2.2 #1608

merged 3 commits into from
May 7, 2020

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented Apr 29, 2020 •
edited
Loading

When using the Khronos OpenCL headers from GitHub, set the preprocessor
symbol CL_TARGET_OPENCL_VERSION=220 to silence the compilation warning
from the OpenCL header files, and define all OpenCL 2.2 symbols.

Include the OpenCL headers through CL/sycl/detail/cl.h, and define
the CL_TARGET_OPENCL_VERSION to 220 (OpenCL 2.2) by default.

Protect the use of OpenCL 2.0 and 2.2 symbols with the relevant #ifdef.

@fwyzard
Copy link
Contributor Author

fwyzard commented Apr 29, 2020

Fixes #1607.

@fwyzard fwyzard mentioned this pull request Apr 29, 2020
Closed
dm-vodopyanov
dm-vodopyanov previously approved these changes Apr 29, 2020
View reviewed changes
Copy link
Contributor

@dm-vodopyanov dm-vodopyanov left a comment

Choose a reason for hiding this comment

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

LGTM

romanovvlad
romanovvlad previously approved these changes Apr 29, 2020
View reviewed changes
bader
bader reviewed Apr 29, 2020
View reviewed changes
@fwyzard fwyzard dismissed stale reviews from romanovvlad and dm-vodopyanov via 780efe6 April 30, 2020 10:26
fwyzard added 2 commits April 30, 2020 12:27
@fwyzard
[AOT] Define CL_TARGET_OPENCL_VERSION to OpenCL 2.2
d22d2d8 
When using the Khronos OpenCL headers from GitHub, set the preprocessor
symbol CL_TARGET_OPENCL_VERSION=220 to silence the compilation warning
from the OpenCL header files, and define all OpenCL 2.2 symbols.

Signed-off-by: Andrea Bocci <andrea.bocci@cern.ch>
@fwyzard
[SYCL] Define CL_TARGET_OPENCL_VERSION to OpenCL 2.2
909d25c 
When using the Khronos OpenCL headers from GitHub, set the preprocessor
symbol CL_TARGET_OPENCL_VERSION=220 to silence the compilation warning
from the OpenCL header files, and define all OpenCL 2.2 symbols.

Signed-off-by: Andrea Bocci <andrea.bocci@cern.ch>
@fwyzard fwyzard force-pushed the fix_1607 branch 2 times, most recently from 083e0e7 to 2043cd0 Compare April 30, 2020 10:34
@smaslov-intel
Copy link
Contributor

What will it be if someone continues to include <CL/cl.h> or <CL/opencl.h> directly? Will we just see the warning again? Can we somehow guard from doing this? Maybe add an error directive into some common SYCL header to check that CL_TARGET_OPENCL_VERSION is defined when CL is included/defined (there should be something robust to reflect that I believe),

@bader
Copy link
Contributor

bader commented Apr 30, 2020

What will it be if someone continues to include <CL/cl.h> or <CL/opencl.h> directly? Will we just see the warning again? Can we somehow guard from doing this? Maybe add an error directive into some common SYCL header to check that CL_TARGET_OPENCL_VERSION is defined when CL is included/defined (there should be something robust to reflect that I believe),

IIRC, it's not a warning - it's a remark.

@bader
Copy link
Contributor

bader commented Apr 30, 2020

It's a "pragma message" - https://github.com/KhronosGroup/OpenCL-Headers/blob/master/CL/cl_version.h#L22

bader
bader reviewed Apr 30, 2020
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

LGTM.
A few nits.

@fwyzard
[SYCL] Default to the OpenCL 2.2 specification level
6ab6055 
Include all OpenCL headers through CL/sycl/detail/cl.h, and define
the CL_TARGET_OPENCL_VERSION to 220 (OpenCL 2.2) by default.

Signed-off-by: Andrea Bocci <andrea.bocci@cern.ch>
bader
bader previously approved these changes Apr 30, 2020
View reviewed changes
@smaslov-intel
Copy link
Contributor

What will it be if someone continues to include <CL/cl.h> or <CL/opencl.h> directly? Will we just see the warning again? Can we somehow guard from doing this? Maybe add an error directive into some common SYCL header to check that CL_TARGET_OPENCL_VERSION is defined when CL is included/defined (there should be something robust to reflect that I believe),

IIRC, it's not a warning - it's a remark.

OK, but the point is that can we make something to get an error instead?

@fwyzard
Copy link
Contributor Author

fwyzard commented Apr 30, 2020

What will it be if someone continues to include <CL/cl.h> or <CL/opencl.h> directly? Will we just see the warning again?

Actually, I don't think we would see anything during the build, because CMake now does define CL_TARGET_OPENCL_VERSION.
The message could potentially happen in some user's code that includes the SYCL header.

Can we somehow guard from doing this? Maybe add an error directive into some common SYCL header to check that CL_TARGET_OPENCL_VERSION is defined when CL is included/defined

Mhm... the most robust thing I can think of would be to patch the version of CL/cl_version.h to replace the #pragma message with an #error - but I don't think this change should be included in the version that is installed.

An other possibility could be some check like

#if defined __CL_VERSION_H && ! defined CL_TARGET_OPENCL_VERSION
#error Do not include <CL/cl.h> or <CL/opencl.h> directly, use <CL/sycl/detail/cl.h> instead
#endif

but I have no idea how one could apply to all relevant cpp files.

@fwyzard
Copy link
Contributor Author

fwyzard commented Apr 30, 2020

One of the check fails with

The following symbols are missing from the new object file:
_ZN2cl4sycl7handler15addEventToQueueESt10shared_ptrINS0_6detail10queue_implEENS0_5eventE
error: command failed with exit status: 255

but in fact I can't find any reference to it - maybe it's related to #1622 ?

smaslov-intel
smaslov-intel previously approved these changes May 1, 2020
View reviewed changes
@fwyzard fwyzard dismissed stale reviews from smaslov-intel and bader via 6ab6055 May 1, 2020 09:40
@fwyzard
Copy link
Contributor Author

fwyzard commented May 1, 2020

I've reverted the opencl-aot code changes.
Could you retrigger the checks ?

@bader bader requested a review from smaslov-intel May 1, 2020 12:49
@fwyzard
Copy link
Contributor Author

fwyzard commented May 6, 2020

@bader could you trigger the Lit_With_Cuda test ?

@bader bader merged commit 8a0fbfb into intel:sycl May 7, 2020
bb-sycl pushed a commit that referenced this pull request Sep 15, 2022
@vmaksimo @dbudanov-cmplr
Translate function pointer from global variable as pointer, not as de…
e91c9da 
…claration (#1608)

This patch helps to avoid invalid SPV generation.

When global variable contains a pointer to a function, translator tries to
translate it as declaration. Then it translates this function the second time
when going through the function list. This leads to double translation
of the same function and to the usage of the same IDs in SPIR-V file.

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@2b4ce42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader approved these changes

@dm-vodopyanov dm-vodopyanov dm-vodopyanov approved these changes

@romanovvlad romanovvlad romanovvlad approved these changes

@AlexeySachkov AlexeySachkov Awaiting requested review from AlexeySachkov

@jbrodman jbrodman Awaiting requested review from jbrodman

@smaslov-intel smaslov-intel Awaiting requested review from smaslov-intel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fwyzard @smaslov-intel @bader @dm-vodopyanov @romanovvlad