-
Notifications
You must be signed in to change notification settings - Fork 113
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.
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
Remove usage of deprecated CL/sycl.hpp header and deprecated ::cl::sycl namespace #431
Conversation
@@ -90,7 +90,7 @@ Random number generation is available for |dpcpp_long| device-side and host-side | |||
|
|||
#include <iostream> | |||
#include <vector> | |||
#include <CL/sycl.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DPC++ compiler which is available as a part of Intel® oneAPI toolkits (you can download it e.g. here) does not contain <sycl/sycl.hpp>. The solution is:
#if __has_include(<sycl/sycl.hpp>)
#include <sycl/sycl.hpp>
#else
#include <CL/sycl.hpp>
#endif
However, having such code in a sample is not good idea: it should be as simple as possible. The idea of omitting all the headers is not good either, because we should show which oneDPL headers to include. Having only oneDPL headers will look strange due to incompleteness of the samples. I suggest using <CL/sycl.hpp>. The comment applies to all the samples from documentation/library_guide
and examples
folders.
@MikeDvorskiy, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting for final decision on this one. I'd like to point out that this is not just a matter of taste; for any other compliant SYCL compiler apart from DPC++ users following the example may get compiler errors if CL/sycl.hpp
is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, "DPC++ compiler and libraries" does not contain <sycl/sycl.hpp> yet.
Can we distinguish SYCL compiler apart from DPC++ by a compiler version macro?
In that case we can write something like that:
#if defined(__INTEL_LLVM_COMPILER) && SYCL_LANGUAGE_VERSION < "version_value"
#include <CL/sycl.hpp>
#else
#include <sycl/sycl.hpp>
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the examples. I think we may(or even must) omit <sycl/sycl.hpp>
in the examples and other user's code which uses OneDPL, because the SYCL main header should be included once, within OneDPL library. For example, here https://github.com/oneapi-src/oneDPL/blob/main/include/oneapi/dpl/pstl/hetero/dpcpp/sycl_defs.h#L24
So, when a user includes any OneDPL public header, he gets SYCL library inclusion implicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MikeDvorskiy, GitHub version of DPC++ has both <sycl/sycl.hpp>
and <CL/sycl.hpp>
while DPC++ supplied as a part of Intel® oneAPI toolkits has only <CL/sycl.hpp>
.
What is benefit of using:
#if defined(__INTEL_LLVM_COMPILER) && SYCL_LANGUAGE_VERSION < "version_value"
#include <CL/sycl.hpp>
#else
#include <sycl/sycl.hpp>
#endif
instead of:
#if __has_include(<sycl/sycl.hpp>)
#include <sycl/sycl.hpp>
#else
#include <CL/sycl.hpp>
#endif
?
Meanwhile, the second approach does not require specifying version of the macro and its uplifting if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed the thread with @MikeDvorskiy offline. We agreed on the following:
- Get rid of inclusion of
sycl.hpp
in the examples and the guide. - Use the approach with
__has_include
insycl_defs.h
.
b345061
to
7c6fe8a
Compare
7c6fe8a
to
4e5f0ee
Compare
As the issue has been already addressed (see #403), I am closing this PR. Anyway, thank you, @illuhad. The adjustments made separately were inspired by this PR. If you wish to be mentioned as a contributor, create a PR with the changes in https://github.com/oneapi-src/oneDPL/blob/main/CREDITS.txt. It may have a note like this one: "Identified the issue with non-compliant namespaces and headers (SYCL 2020), and submitted an extensive change request, which served as a basis for the finalized solution.". |
Following the discussion in #403, this PR
CL/sycl.hpp
header that has been deprecated in SYCL 2020::cl::sycl
namespace that has been deprecated in SYCL 2020Fixes #403