Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Port std::span and enable if for C++11 onwards to support mdspan #313

Merged
merged 1 commit into from
Nov 14, 2022
Merged

Port std::span and enable if for C++11 onwards to support mdspan #313

merged 1 commit into from
Nov 14, 2022

Conversation

miscco
Copy link
Collaborator

@miscco miscco commented Sep 13, 2022 •
edited
Loading

In addition to porting the span implementation from libc++ the PR does the following:

  • Remove outdated support for tuple interface
  • Remove outdated support for const_iterator
  • Change index_type to size_type

I intentionally did not adopt the ranges support, as that is out of scope.

@miscco miscco requested a review from wmaxey September 13, 2022 12:34
@miscco
Copy link
Collaborator Author

miscco commented Sep 13, 2022

Notably, this does not include any of the ranges work. We can only add that once we added ranges support.

@wmaxey wmaxey assigned wmaxey and unassigned wmaxey Sep 13, 2022
@miscco miscco force-pushed the span branch 3 times, most recently from 14a9ba4 to 514600c Compare September 13, 2022 19:32
jrhemstad
jrhemstad reviewed Sep 14, 2022
View reviewed changes
Copy link
Collaborator

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

It seems like there are more changes to the upstream implementation than I'm used to seeing for porting a header for libc++. Could you update the PR description with a summary of what needed to be changed?

@miscco
Copy link
Collaborator Author

miscco commented Sep 14, 2022

It seems like there are more changes to the upstream implementation than I'm used to seeing for porting a header for libc++. Could you update the PR description with a summary of what needed to be changed?

Yeah, <span> underwent a ton of last minute changes. If I remember the last 3 standards meetings where quite loaded with changes. That involved:

  • Removing tuple support
  • Removing const_iterator support
  • Changing index_type to size_type
  • Ranges support

I intentionally did not adopt any ranges changes, as we do not have a ranges implementation working yet.

@jrhemstad
Copy link
Collaborator

Yeah, <span> underwent a ton of last minute changes.

Ah, so this is because our upstream copy of libc++ is a bit old. That makes more sense.

Would it be possible/sane to just checkout the span related files and tests from upstream libc++? Or is that effectively what you did?

@miscco
Copy link
Collaborator Author

miscco commented Sep 14, 2022

Yeah, <span> underwent a ton of last minute changes.

Ah, so this is because our upstream copy of libc++ is a bit old. That makes more sense.

Would it be possible/sane to just checkout the span related files and tests from upstream libc++? Or is that effectively what you did?

Yes and no. I went ahead and ported all fixes that apply to us. However, the container interface has changed drastically due to ranges support, so it is not a 100% port.

@wmaxey
Copy link
Member

wmaxey commented Sep 14, 2022 •
edited
Loading

Should we consider keeping a copy of the original libcxx tests in libcxx/test/std/...?

I think it's an important piece of coverage.

@miscco
Copy link
Collaborator Author

miscco commented Sep 15, 2022

Should we consider keeping a copy of the original libcxx tests in libcxx/test/std/...?

I think it's an important piece of coverage.

I did not test the old tests, some of them will definitely fail, like those for the tuple interface.

So I guess we should either remove them or adopt them to the new state of things

@wmaxey
Copy link
Member

wmaxey commented Sep 15, 2022 •
edited
Loading

I did not test the old tests, some of them will definitely fail, like those for the tuple interface.
So I guess we should either remove them or adopt them to the new state of things

We could skip interfaces we don't support. 🤔

griwes
griwes approved these changes Sep 27, 2022
View reviewed changes
Copy link
Collaborator

@griwes griwes left a comment

Choose a reason for hiding this comment

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

LGTM overall, but please address the two comments.

include/cuda/std/detail/libcxx/include/span Show resolved Hide resolved
include/cuda/std/detail/libcxx/include/span Outdated Show resolved Hide resolved
miscco
miscco commented Sep 27, 2022
View reviewed changes
include/cuda/std/detail/libcxx/include/span

#if defined(_LIBCUDACXX_USE_PRAGMA_GCC_SYSTEM_HEADER)
#pragma GCC system_header
#endif

_LIBCUDACXX_BEGIN_NAMESPACE_STD

#if _LIBCUDACXX_STD_VER > 17
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I remember correctly, mdspan is C++14 only, so maybe we want to restrict this also to C++14?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I don't think we need to go through any efforts to backport this (or anything else) to C++11.

include/cuda/std/detail/libcxx/include/span Outdated Show resolved Hide resolved
include/cuda/std/detail/libcxx/include/span Show resolved Hide resolved
@youyu3
Copy link
Contributor

youyu3 commented Sep 30, 2022

Need to update include/cuda/std/detail/libcxx/include/version and define __cpp_lib_span. Otherwise, this worked for my mdspan tests. Thanks!

@wmaxey
Copy link
Member

wmaxey commented Sep 30, 2022

Will run tests and merge if no issues occur.

@wmaxey
Copy link
Member

wmaxey commented Sep 30, 2022

Need to update include/cuda/std/detail/libcxx/include/version and define __cpp_lib_span. Otherwise, this worked for my mdspan tests. Thanks!

Should that be addressed in this PR?

@griwes
Copy link
Collaborator

griwes commented Sep 30, 2022

It should.

@vyasr vyasr mentioned this pull request Oct 19, 2022
Merged
3 tasks
@miscco miscco force-pushed the span branch 2 times, most recently from 4b08898 to 0097f34 Compare October 25, 2022 06:47
@miscco miscco force-pushed the span branch 2 times, most recently from f917c15 to 45e6070 Compare November 3, 2022 08:00
@miscco miscco force-pushed the span branch 3 times, most recently from e021960 to e68b1b9 Compare November 8, 2022 13:42
wmaxey
wmaxey reviewed Nov 8, 2022
View reviewed changes
.upstream-tests/test/std/views/views.span/span.cons/span.fail.cpp
//
//===----------------------------------------------------------------------===//
// UNSUPPORTED: c++03, c++11

Copy link
Member

Choose a reason for hiding this comment

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

.fail tests do not yet support NVRTC.

wmaxey
wmaxey reviewed Nov 8, 2022
View reviewed changes
.upstream-tests/test/std/views/views.span/span.cons/iterator_len.verify.cpp
//
//===---------------------------------------------------------------------===//
// UNSUPPORTED: c++03, c++11

Copy link
Member

@wmaxey wmaxey Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

.verify tests probably also do not support NVRTC, but CI shows them passing somehow.

@miscco miscco force-pushed the span branch 2 times, most recently from b6608b4 to 4c3bc98 Compare November 11, 2022 14:35
@miscco miscco added the testing: internal ci passed Passed internal NVIDIA CI (DVS). label Nov 13, 2022
@miscco
Port std::span and enable if for C++11 onwards to support mdspan
f98f43f 
Notably this does not take the fairly outdated state of our libc++ fork, but upgrades
it to the actually approved state that became C++20.

The only missing pieces are ranges support, but we can happily do that once we support ranges.
@miscco miscco merged commit e7cddb1 into NVIDIA:main Nov 14, 2022
@miscco miscco deleted the span branch November 14, 2022 07:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@griwes griwes griwes approved these changes

@wmaxey wmaxey wmaxey approved these changes

@jrhemstad jrhemstad jrhemstad approved these changes

Assignees
No one assigned
Labels
testing: internal ci passed Passed internal NVIDIA CI (DVS).
Projects
CCCL
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@miscco @jrhemstad @wmaxey @youyu3 @griwes