Rework of submdspan for layout_left/right #337
Conversation
|
Note this PR includes the changes from #336 |
| template<class IntegralType> | ||
| MDSPAN_TEMPLATE_REQUIRES( | ||
| class IntegralType, | ||
| // The is_convertible condition is added to make sfinae valid |
| std::tuple{detail::stride_of(slices)...})), | ||
| #endif | ||
| offset | ||
| }; | ||
| } | ||
| #if defined(__NVCC__) && !defined(__CUDA_ARCH__) && defined(__GNUC__) | ||
| __builtin_unreachable(); |
There was a problem hiding this comment.
Tangent: This GCC built-in works fine for us in device code (with __CUDA_ARCH__ defined).
| deduce_layout::layout_left_value, layout_left, | ||
| std::conditional_t< | ||
| deduce_layout::layout_left_padded_value, | ||
| MDSPAN_IMPL_PROPOSED_NAMESPACE::layout_left_padded<dynamic_extent>, |
There was a problem hiding this comment.
I might be missing something, but does this not implement the layout_left_padded with static padding stride case? That should be possible, as in the following example.
auto mapping = layout_left::mapping<extents<int, 10, 10>>{};
auto x_storage = std::array<float, 100>{};
auto x = mdspan{x_storage.data(), mapping};
auto slice = strided_slice{
.offset=0,
.extent=integral_constant<int, 5>{},
.stride=integral_constant<int, 1>{}};
// Should be `layout_left_padded<10>::mapping<extents<int, 5, 5>>`
auto x_sub = submdspan(x, strided_slice, full_extent);Does the wording not permit this, or is that just future work? Totally OK if so.
There was a problem hiding this comment.
The standard actually does not allow this. I.e. according to the standard you get layout_stride. I am not sure I want to change that, i.e. explicitly treating the strided_slice with integral_constant 1 for stride like a pair range. I mean we could if we really wanted to, not sure it wins us much. And it might be weird in generic code to get layout_stride vs layout_left or so back depending on an integral constant. In all other cases the layout_flavor you get really just depends on the slice flavors you have, not the particular type of slice.
That said: this PR always returns dynamic_extent for the padding value when it returns padded layouts. I am fixing that in the follow up which actually introduces submdspan for padded layouts. That anyway requires a bunch of more extensive new tests.
There was a problem hiding this comment.
I see -- so the thing we still need to implement is https://eel.is/c++draft/views.multidim#mdspan.sub.map.left-1.4.5 , that defines S_static as "the product of all values static_extent(k) for k in the range [0, u + 1)" in case all the extents in the range are static?
There was a problem hiding this comment.
Yes, I am doing it as part of the padded layouts submdspan. Should be done later today or tomorrow.
There was a problem hiding this comment.
The nice thing is that our tests are already comprehensive enough to cover those cases (i.e. getting a static stride and getting a dynamic stride in various cases).
Fix typos Co-authored-by: Mark Hoemmen <[email protected]>
crtrott
force-pushed
the
padded-layouts-submdspan
branch
from
a853e4d
to
f79db0b
Compare
There was a problem hiding this comment.
I did a first pass. The logic is a bit hard to follow and took me a while to get. That's partly because the slicing logic is complex but also because of the way the logic is expressed; I think it would be better to write some compile-time functions to initialize the variables that decide which layout type to use.
Also might even help readability to have an enum instead of two booleans and use that to select the type from a specialization
| IndexType, SubRank, std::index_sequence<Idx...>, SliceSpecifiers...> { | ||
|
|
||
| using count_range = index_sequence_scan_impl< | ||
| 0, (is_index_slice_v<SliceSpecifiers, IndexType> ? 0 : 1)...>; |
There was a problem hiding this comment.
| 0, (is_index_slice_v<SliceSpecifiers, IndexType> ? 0 : 1)...>; | |
| 0, (is_range_slice_v<SliceSpecifiers, IndexType> ? 1 : 0)...>; |
Might be clearer
There was a problem hiding this comment.
No, there are slice specifiers which are neither (strided)
There was a problem hiding this comment.
I guess we should rename this to count_non_index or count_range_or_strided then
|
Does this require a changelog entry for 4.4? |
|
Oop sorry for noise about changelog, saw this listed in the Kokkos Planning board |
This enables submdspan of layout_left and layout_right to create
layout_left_paddedandlayout_right_paddedrespectively.Actual submdspan implementation for the padded layouts will come in a subsequent PR.