oneDPL icon indicating copy to clipboard operation
oneDPL copied to clipboard

CL/sycl.hpp and ::sycl namespace usage

Open illuhad opened this issue 3 years ago • 2 comments

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. https://github.com/KhronosGroup/SYCL-CTS/pull/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?

illuhad avatar Oct 29 '21 17:10 illuhad

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.

akukanov avatar Oct 29 '21 19:10 akukanov

@akukanov Lovely, I will create a PR!

illuhad avatar Oct 29 '21 19:10 illuhad

@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.

dmitriy-sobolev avatar Jan 10 '23 11:01 dmitriy-sobolev