Skip to content
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

CL/sycl.hpp and ::sycl namespace usage #403

Closed
illuhad opened this issue Oct 29, 2021 · 3 comments · May be fixed by #431
Closed

CL/sycl.hpp and ::sycl namespace usage #403

illuhad opened this issue Oct 29, 2021 · 3 comments · May be fixed by #431

Comments

@illuhad
Copy link

illuhad commented Oct 29, 2021

When rebasing my changes to support hipSYCL on the latest oneDPL, I noticed the following issue that I would like to get guidance on w.r.t. how I can best fix this upstream.

oneDPL and all tests include CL/sycl.hpp, but expect that SYCL objects are exposed in ::sycl. This is the case in DPC++, but I believe that this is neither guaranteed nor demanded by SYCL 2020. The relevant section is 4.3 of the SYCL 2020 spec:

SYCL provides one standard header file: <sycl/sycl.hpp>, which needs to be included in every translation unit that uses the SYCL programming API.
All SYCL classes, constants, types and functions defined by this specification should exist within the ::sycl namespace.
For compatibility with SYCL 1.2.1, SYCL provides another standard header file: <CL/sycl.hpp>, which can be included in place of <sycl/sycl.hpp>. In that case, all SYCL classes, constants, types and functions defined by this specification should exist within the ::cl::sycl C++ namespace.

My reading of this is that, when CL/sycl.hpp is used, everything should be exclusively in ::cl::sycl (after all, also exposing ::sycl would unnecessarily pollute the global namespace). As such, hipSYCL only exposes ::sycl when including the new header sycl/sycl.hpp.

This issue is also discussed in the Khronos SYCL WG, see e.g. KhronosGroup/SYCL-CTS#108

Since any changes to fix this potentially affect a lot of files in oneDPL, I'd like some guidance how this can best be resolved. My ideas are:

  • Does DPC++ already support the sycl/sycl.hpp header? If so, changing all includes to this header would be best, as this is the official SYCL 2020 header.
  • Alternatively we could special case all includes to include sycl/sycl.hpp when using hipSYCL
  • Or add some using namespace ::cl somewhere when using hipSYCL?
@akukanov
Copy link
Contributor

Yes, DPC++ should have the new header: https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/sycl.hpp
I agree that changing oneDPL to use the new header is the right way to go.

@illuhad
Copy link
Author

illuhad commented Oct 29, 2021

@akukanov Lovely, I will create a PR!

@dmitriy-sobolev
Copy link
Contributor

@illuhad, oneDPL does not use cl:: namespace nor CL/sycl.hpp header now. It has been fixed in PR #722 and #631. Let me close that issue. Feel free to reopen it if something still needs to be done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants