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

Implement P0009R18 <mdspan> et al. #3502

Conversation

MattStephanson
Copy link
Contributor

@MattStephanson MattStephanson commented Feb 25, 2023

I've been working on this for a while, but it's gotten to the point where if I hold it until it's fully implemented and tested, there won't be much time left to benchmark. Particularly for extents and layout_stride, there are different choices you could make about how to store the extents and strides that will affect codegen. If we want to investigate alternatives, it has to be done before ABI is locked. So @StephanTLavavej suggested I just submit what I have to a feature branch. I hope it's a useful starting point.

I think extents, layout_left, and layout_right are in the best shape. When there are constraints on constructors, I've tried to set up tests that each fail exactly one constraint. However, I've lately become doubtful about is_constructible due to https://godbolt.org/z/q1M46rf6j. The last two static_asserts fail, yet the corresponding definitions just above them don't compile. I've been replacing some of the positive is_constructible tests with actual definitions, but it's a work in progress.

Other things that I know are not tested thoroughly:

  • index_type vs. size_type vs. rank_type vs. size_t.
  • extended integer types should be allowed
  • correct behavior for rank-zero spaces (equivalent to a scalar) and spaces that have non-zero rank but are empty because at least one extent is zero.

As for optimization, these are what I would look at if I had time:

  • How to access the static extents. I've chosen to store them similar to a std::array, as a base class with a special case for zero dimensions, but without the indirection and debug codegen of using as actual std::array. Other options I can think of are just using a std::array for simplicity, use one of the various algorithms for extracting the n-th member of a parameter pack, or use a recursive base class scheme like tuple.
  • The same goes for layout_stride. Here I just used a std::array, to get the implementation done as quickly as possible.
  • Where an index_sequence is used, sometimes I pass it to a function template and sometimes to a generic lambda. The lambda could cause code bloat if it's treated like an anonymous namespace, with internal linkage. But I think that identical instantiations in different TUs are supposed to be the same type and inline, so the linker will discard the duplicates. If that's not the case, it might be better to convert everything to function templates.
  • Fold expressions vs. for loops. I've usually preferred the latter for clarity, but you could probably convert any of them to a fold expression, possibly by introducing an index_sequence. I have no intution, though, about whether that would be better, worse, or about the same.

I still hope to be involved, but it will likely have to be on narrow issues, as and when time permits.

@MattStephanson MattStephanson requested a review from a team as a code owner February 25, 2023 04:20
@StephanTLavavej StephanTLavavej added cxx23 C++23 feature mdspan C++23 mdspan labels Feb 25, 2023
@StephanTLavavej StephanTLavavej merged commit 69751ad into microsoft:feature/mdspan2 Feb 25, 2023
@frederick-vs-ja
Copy link
Contributor

  • extended integer types should be allowed

I think this is a non-goal, since extended integer types are basically unsupported elsewhere in MSVC STL.

template <class... _OtherIndexTypes,
enable_if_t<((sizeof...(_OtherIndexTypes) > 0)) && (is_convertible_v<_OtherIndexTypes, index_type> && ...)
&& (is_nothrow_constructible_v<index_type, _OtherIndexTypes> && ...)
//&& (sizeof...(_OtherIndexTypes) == rank() || sizeof...(_OtherIndexTypes) == rank_dynamic())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is commented out because it breaks one of the tests,

static constexpr int arr[6] = {};
constexpr mdspan<const int, extents<size_t, dynamic_extent, 3>> mds1(arr, 2);

but I haven't been able to figure out why.

@MattStephanson MattStephanson deleted the ill_have_your_mdspan_i_love_it branch May 10, 2023 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx23 C++23 feature mdspan C++23 mdspan
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants