Skip to content

[WIP] [DO NOT MERGE] add std::span support #3509

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

Closed

Conversation

cperkinsintel
Copy link
Contributor

@cperkinsintel cperkinsintel commented Apr 7, 2021

This PR depends upon the C++17 uplift which is not yet merged ( #3447 )

This PR adds std::span support to SYCL, however it is not yet ready to merge. It still needs tests and there are decisions about files and file management.

The files libcxx_span.hpp, libcxx_config.hpp and libcxx_undefmacros.hpp are all copies from Clang libcxx, which is not presently built or used by DPCPP. The original files were named span, __config and __undef_macros. In the case for libcxx_config.hpp and libcxx_undefmacros.hpp I have only lightly modified them, by converting statements like #include <__config> to #include "libcxx_config.hpp".

But libcxx_span has had more changes, such as dropping the private __wrap_iter class that LIBCXX defines in its implementation of <iterator>. I have inserted comments with //CP wherever I effected a change. Otherwise, I've tried to leave the files untouched - so they are as close to their original LIBCXX counterparts as possible.

Questions:

  • do we want these file names ( libcxx_span.hpp etc) or should they be named something else?
  • is it ok to put these three files in include/CL/sycl ?
  • our SYCL header files use #pragma once but these header files all use the #ifndef X / #define X ... #endif trick. Any issues there?
  • the libcxx_span file opens with a long synopsis comment. I have left that intact.

Anyway, let me know if you have feedback.

@cperkinsintel cperkinsintel changed the title [WIP] add std::span support [WIP] [DO NOT MERGE] add std::span support Apr 7, 2021
@Pennycook
Copy link
Contributor

SYCL 2020 makes std::span available in the sycl:: namespace (see https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#sec:progmodel.futurecppversion). Do you want to do that as part of this PR?

@cperkinsintel
Copy link
Contributor Author

@Pennycook - Yes, it needs to be moved to the sycl namespace and it should check for being available before including this. Thanks for reminding me.

@romanovvlad romanovvlad requested a review from alexbatashev April 8, 2021 16:59
//===---------------------------------------------------------------------===//

#ifndef _LIBCPP_SPAN
#define _LIBCPP_SPAN
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the guard should be renamed as well. Otherwise it can guard away original span from libc++ user might want to include.

@cperkinsintel
Copy link
Contributor Author

I've incorporated the feedback. The C++ uplift macro has been merged, so there is a new final PR for the span support here: #3569

I am closing this draft.

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 this pull request may close these issues.

5 participants