Added new submdspan implementation #223
Conversation
This uses ADL customization point design
| template <class ElementType, class Extents, class LayoutPolicy, | ||
| class AccessorPolicy, class... SliceSpecifiers> | ||
| constexpr auto | ||
| submdspan(const mdspan<ElementType, Extents, LayoutPolicy, AccessorPolicy> &src, | ||
| SliceSpecifiers... slices) { | ||
| const auto sub_map_offset = submdspan_mapping(src.mapping(), slices...); |
There was a problem hiding this comment.
Should we consider forwarding the slice specifiers, as in the code below? It shouldn't make a performance difference for the current slice specifiers (that are all structs of small value types anyway); it's more a specification question.
| template <class ElementType, class Extents, class LayoutPolicy, | |
| class AccessorPolicy, class... SliceSpecifiers> | |
| constexpr auto | |
| submdspan(const mdspan<ElementType, Extents, LayoutPolicy, AccessorPolicy> &src, | |
| SliceSpecifiers... slices) { | |
| const auto sub_map_offset = submdspan_mapping(src.mapping(), slices...); | |
| template <class ElementType, class Extents, class LayoutPolicy, | |
| class AccessorPolicy, class... SliceSpecifiers> | |
| constexpr auto | |
| submdspan(const mdspan<ElementType, Extents, LayoutPolicy, AccessorPolicy> &src, | |
| SliceSpecifiers&&... slices) { | |
| const auto sub_map_offset = submdspan_mapping(src.mapping(), forward<SliceSpecifiers>(slices)...); |
There was a problem hiding this comment.
When I tried this I ran into some trouble with failing tests, i.e. some deduction went haywire, my guess is that all the internal logic also needs to account for && including all the helper functions for matching and counting etc.
There was a problem hiding this comment.
Thanks for trying! I think it's OK to leave this alone for now. If LWG wants it we can investigate it then.
| SliceSpecifiers...>::type; | ||
| }; | ||
|
|
||
| // final specialziation containing the final index_sequence |
There was a problem hiding this comment.
A minor spelling issue:
| // final specialziation containing the final index_sequence | |
| // final specialization containing the final index_sequence |
Does "final specialization" mean "this is the end of the 'recursion' on inv_map_rank specializations"?
There was a problem hiding this comment.
I updated the comment to express this better
| // InvMapRank is gonna be a index_sequence, which we build recursively | ||
| // to contain the mapped indicies. |
There was a problem hiding this comment.
| // InvMapRank is gonna be a index_sequence, which we build recursively | |
| // to contain the mapped indicies. | |
| // InvMapRank is an index_sequence, which we build recursively | |
| // to contain the mapped indices. |
| template <size_t Counter, class InvMapRank, class... SliceSpecifiers> | ||
| struct inv_map_rank; | ||
|
|
||
| // specialization reducing rank by one (i.e. integral slice specifier) |
There was a problem hiding this comment.
| // specialization reducing rank by one (i.e. integral slice specifier) | |
| // specialization reducing rank by one (i.e., integral slice specifier) |
| return r.offset; | ||
| } | ||
|
|
||
| // last_of(slice): getting end of slice sepcifier range |
There was a problem hiding this comment.
| // last_of(slice): getting end of slice sepcifier range | |
| // last_of(slice): getting end of slice specifier range |
| requires( | ||
| !is_convertible_v<Slice, size_t> && | ||
| !is_strided_index_range<Slice>:: | ||
| value) constexpr static auto next_extent(const Extents &ext, |
There was a problem hiding this comment.
Would you consider reformatting to make this more consistent with the other overloads?
| // Return type of submdspan_mapping overloads | ||
| //****************************************** | ||
| template <class Mapping> struct mapping_offset { | ||
| Mapping map; |
There was a problem hiding this comment.
Would you consider calling this mapping instead of map, to avoid collision with std::map?
| Mapping map; | |
| Mapping mapping; |
| struct preserve_layout_left_mapping<index_sequence<Idx...>, SubRank, | ||
| SliceSpecifiers...> { | ||
| constexpr static bool value = | ||
| (SubRank == 0) || | ||
| ((Idx < SubRank - 1 | ||
| ? is_same_v<SliceSpecifiers, full_extent_t> | ||
| : (Idx == SubRank - 1 ? is_same_v<SliceSpecifiers, full_extent_t> || | ||
| is_convertible_v<SliceSpecifiers, | ||
| tuple<size_t, size_t>> | ||
| : true)) && | ||
| ...); | ||
| }; |
There was a problem hiding this comment.
The expression for value is a bit hard to read, but it's also hard to simplify it, given that all the repeated bools in it (like is_same_v<SliceSpecifiers, full_extent_t>) depend on a pack. Rewriting them into tuples or arrays somehow makes it worse.
Would you consider changing the term
Idx == SubRank - 1 ? is_same_v<SliceSpecifiers, full_extent_t> ||
is_convertible_v<SliceSpecifiers,
tuple<size_t, size_t>>
: true)into the following?
Idx != SubRank - 1 or
is_same_v<SliceSpecifiers, full_extent_t> or
is_convertible_v<SliceSpecifiers, tuple<size_t, size_t>>There was a problem hiding this comment.
actually that expression is wrong ...
this thing will return true for integers and what not.
There was a problem hiding this comment.
I inserted a correct one which is more structured like this instead of nested ?: statements.
for strided_index_range.
…output-file fix kokkos#223: cmake ambigous output file name
This uses ADL customization point design.
It doesn't use the backward compatibility stuff yet to improve readability for an initial review, i.e. it will only work on host with C++20 and C++23.
It also doesn't implement yet correct stride calculation when using strided_index_range