kokkos / mdspan

Reference implementation of mdspan targeting C++23
Other
417 stars 69 forks source link

Array indexing of `mdspan` #123

Closed Mick235711 closed 2 years ago

Mick235711 commented 2 years ago

I found an interesting poll in the revision history of P0009:

image

There seems to be consensus in LEWG for allowing static-extent span indexing... except that change seems lost and never implemented in P0009.

Actually, if we want maximum flexibility, we can support the following index type:

  1. array<[const?] SizeType, N> with N == rank()
  2. span<const SizeType, N> with N == rank(). (We cannot make this a template, so basically span<const size_type, Extent::rank()> probably)
  3. tuple<SizeTypes...> with is_integral_v<SizeTypes> && ... and sizeof...(SizeTypes) == rank()
  4. Anything tuple-like (P2165R3) with size == rank() (this subsumes 1 and 3 actually)
  5. {1, 2, 3, 4} (braced-init-list). This can be supported by 2 if P2447 is adopted. (Not really, initializer_list<int> cannot convert to span<const size_t> afaik, and even if it does, convert to a static-extent span is explicit)
  6. 1D mdspan (mdspan<SizeType, extents<N>>)... Is this useful? I don't see the usage, but LEWG above seems to have weak consensus.
  7. contiguous_range? (If this, then subsumes 1, 2, and 5)

My thoughts: 1 is definitely worth support since it is natural to pass an array for indexing. 5 seems useful (ms[{1, 2, 3, 4}]), but supporting this only (through initializer_list) does not seem worth it if we accept 2 and wait for P2447. As for 3, if we want to support this, then we definitely should propose 4 instead, since it is a trend to accept tuple-like types (tuple, pair, array) anywhere tuple is accepted. As for 7, this is weird, but maybe it have its own use case since range is a general term.

2 don't seems generally useful. A span of static extent cannot be constructed from things like vector, so it has only limited usage.

Therefore, my own opinion is to choose from either (from above thoughts, 3 (-> 4) is not worth it to support alone, and if we support 2 probably we need to support 5? and 6... I will not list it but it can be added to any of below)

  1. Support 1 only (status quo)
  2. Support 1 and 5 (and 2)
  3. Support 4 (subsumes 1)
  4. Support 4 and 5 (and 2)... this is most complete, but is it worth it?

I'm not really sure what to do here, since as the Against vote had said, I don't see use case for span as index either... array and tuple as index seems fairly useful, but I'm not sure if it is worth it.

My personal recommendation is to support 4 & 5, and consider 2 (or 6) if we want to follow LEWG. However I'm not very firmly against other choices.

crtrott commented 2 years ago

This was dropped later, in the process (I mean that vote was like 4 years ago) and basically the team behind mdspan said that indexing with a span is kinda not useful since you essentially inject multiple additional memory dereferences for each actually intended memory access. Somehow, we didn't fully consider that issue when we voted on allowing span. Furthermore, as you noted we didn't realize some of the limitations of span which make the usecase even less useful (such explicit construction).

Also note that R14 of mdspan was approved by LEWG and this is now in wording review in LWG. Since in principle the design freeze for C++23 is already here, any additional changes in time for C++23 have an even higher bar to make it in, and thus will likely just focus on stuff which is more like a defect.

Consequently, we will live with status quo unless someone has a very compelling argument to change this and puts in a paper maybe with an accompanying national body comment.

Mick235711 commented 2 years ago

basically the team behind mdspan said that indexing with a span is kinda not useful since you essentially inject multiple additional memory dereferences for each actually intended memory access

I think that arise with the current array overload too?

basically the team behind mdspan said that indexing with a span is kinda not useful since you essentially inject multiple additional memory dereferences for each actually intended memory access

Of course, this is just a preliminary discussion and I am by no means want to block P0009 or change it. Adding an operator[] is a non-breaking change (I presume) and more suitable for a C++26 follow up paper. I'm just discussing some possible options.❤️

crtrott commented 2 years ago

The array overload does not necessarily have that problem since the array owns static storage i.e. a size_t[N], which the compiler can put into registers. So generally std::array is less problematic from that perspective than std::span.

Note also that we already are using [] operator and not () in the proposal. Our implementation does support both, though none of the main line compilers implement [] yet. There is an experimental compiler in godbolt which can do [] already, though it fails for zero arguments - hence our tests use a macro to work around that issue.

Mick235711 commented 2 years ago

The array overload does not necessarily have that problem since the array owns static storage i.e. a size_t[N], which the compiler can put into registers. So generally std::array is less problematic from that perspective than std::span.

That's a new perspective. Thanks! I think that may alone reject 3, 6 and 7. Maybe I need to do some assembly inspection before writing a proposal for this...

Note also that we already are using [] operator and not () in the proposal.

Yeah, I know that (and btw, GCC 12/Clang 15 seems to implement it already, so there is hope). My original suggestion is to replace the current

template<class SizeType, size_t N>
    constexpr reference operator[](const array<SizeType, N>& indices) const; // (1)

with something like

template<tuple-like T>
    constexpr reference operator[](const T& indices) const; // (4)
template<typename T> // optional (5)
    constexpr reference operator[](const std::initializer_list<T>& indices) const; // optional (5)

(Does not seem particularly necessary... maybe this need more thoughts)

crtrott commented 2 years ago

Update: LWG did want the span indexing, so we have that now ...

mhoemmen commented 2 years ago

Closing because the desired feature was added. Please feel free to reopen if this is not the case. Thanks!