On Thu, May 29, 2025 at 9:23 AM Tomasz Kaminski <tkami...@redhat.com> wrote:
> > > > On Mon, May 26, 2025 at 4:13 PM Luc Grosheintz <luc.groshei...@gmail.com> > wrote: > >> Implements the remaining parts of layout_left and layout_right; and all >> of layout_stride. >> >> The implementation of layout_stride::mapping::is_exhaustive applies >> the following change to the standard: >> >> 4266. layout_stride::mapping should treat empty mappings as exhaustive >> >> https://cplusplus.github.io/LWG/issue4266 >> >> libstdc++-v3/ChangeLog: >> >> * include/std/mdspan(layout_stride): New class. >> * src/c++23/std.cc.in: Add layout_right. >> >> Signed-off-by: Luc Grosheintz <luc.groshei...@gmail.com> >> --- >> > There are some more meaningful suggestions below: > * document what precondition are checked in commit message, > I have suggested adding a few > * handle empty index space in __offset, I would suggest also having > __empty helper for > extents (we will use it in mdspan). I think we could also use it in > __representable_size. > * is_always_exhaustive should also check for rank 0 > > libstdc++-v3/include/std/mdspan | 229 ++++++++++++++++++++++++++++++- >> libstdc++-v3/src/c++23/std.cc.in | 3 +- >> 2 files changed, 230 insertions(+), 2 deletions(-) >> >> diff --git a/libstdc++-v3/include/std/mdspan >> b/libstdc++-v3/include/std/mdspan >> index 7daa0713716..d5f613a19fd 100644 >> --- a/libstdc++-v3/include/std/mdspan >> +++ b/libstdc++-v3/include/std/mdspan >> @@ -403,6 +403,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> class mapping; >> }; >> >> + struct layout_stride >> + { >> + template<typename _Extents> >> + class mapping; >> + }; >> + >> namespace __mdspan >> { >> template<typename _Tp> >> @@ -496,7 +502,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> >> template<typename _Mapping> >> concept __standardized_mapping = __mapping_of<layout_left, >> _Mapping> >> - || __mapping_of<layout_right, >> _Mapping>; >> + || __mapping_of<layout_right, >> _Mapping> >> + || __mapping_of<layout_stride, >> _Mapping>; >> >> template<typename M> >> concept __mapping_like = requires >> @@ -554,6 +561,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> : mapping(__other.extents(), __mdspan::__internal_ctor{}) >> { } >> >> + template<typename _OExtents> >> + requires (is_constructible_v<extents_type, _OExtents>) >> + constexpr explicit(extents_type::rank() > 0) >> + mapping(const layout_stride::mapping<_OExtents>& __other) >> + : mapping(__other.extents(), __mdspan::__internal_ctor{}) >> + { __glibcxx_assert(*this == __other); } >> + >> constexpr mapping& >> operator=(const mapping&) noexcept = default; >> >> @@ -684,6 +698,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> : mapping(__other.extents(), __mdspan::__internal_ctor{}) >> { } >> >> + template<class _OExtents> >> + requires (is_constructible_v<extents_type, _OExtents>) >> + constexpr explicit(extents_type::rank() > 0) >> + mapping(const layout_stride::mapping<_OExtents>& __other) noexcept >> + : mapping(__other.extents(), __mdspan::__internal_ctor{}) >> + { __glibcxx_assert(*this == __other); } >> + >> constexpr mapping& >> operator=(const mapping&) noexcept = default; >> >> @@ -757,6 +778,212 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> [[no_unique_address]] _Extents _M_extents{}; >> }; >> >> + namespace __mdspan >> + { >> + template<typename _Mapping> >> + constexpr typename _Mapping::index_type >> + __offset(const _Mapping& __m) noexcept >> + { >> + using _IndexType = typename _Mapping::index_type; >> + constexpr auto __rank = _Mapping::extents_type::rank(); >> + >> + if constexpr (__standardized_mapping<_Mapping>) >> + return 0; >> + else >> + { >> > If the size of multidimensional index space is zero we cannot call mapping > at all, > and standard recognize it here: > https://eel.is/c++draft/mdspan.layout.stride#expo-1.2 > > otherwise 0, if the size of the multidimensional index space e is 0, > So we need two additional if in between: > else if constexpr (contains_zero(_static_exts<Mapping::extents_type>())) > return 0; > else if (contais_zero(_dyn_exts(_m.extents)) > return 0; > else > >> + auto __impl = [&__m]<size_t... >> _Counts>(index_sequence<_Counts...>) >> + { return __m(((void) _Counts, _IndexType(0))...); }; >> + return __impl(make_index_sequence<__rank>()); >> + } >> + } >> + >> + template<typename _Mapping, typename... _Indices> >> + constexpr typename _Mapping::index_type >> + __linear_index_strides(const _Mapping& __m, >> + _Indices... __indices) >> > Maka this noexcept, because it is called from noexcept function. > >> + { >> + using _IndexType = typename _Mapping::index_type; >> + _IndexType __res = 0; >> + if constexpr (sizeof...(__indices) > 0) >> + { >> + auto __update = [&, __pos = 0u](_IndexType __idx) mutable >> + { >> + __res += __idx * __m.stride(__pos++); >> + }; >> + (__update(__indices), ...); >> + } >> + return __res; >> + } >> + } >> + >> + template<typename _Extents> >> + class layout_stride::mapping >> + { >> + public: >> + using extents_type = _Extents; >> + using index_type = typename extents_type::index_type; >> + using size_type = typename extents_type::size_type; >> + using rank_type = typename extents_type::rank_type; >> + using layout_type = layout_stride; >> + >> + static_assert(__mdspan::__representable_size<_Extents, index_type>, >> + "The size of extents_type must be representable as index_type"); >> + >> + constexpr >> + mapping() noexcept >> + { >> > The standard has a precondition here > https://eel.is/c++draft/mdspan.layout.stride#cons-1: > > *Preconditions*: layout_right::mapping<extents_type>().required_span_ > size() is representable as a value of type index_type ([basic.fundamental] > <https://eel.is/c++draft/basic.fundamental>). > <https://eel.is/c++draft/mdspan.layout.stride#cons-1.sentence-1> > However, for all static_extents this is already checked by static_assert > above.And if at least one extent is dynamic, required span size is zero, > so it is trivially true. Could you add a comment here. > >> + size_t __stride = 1; >> + for (size_t __i = extents_type::rank(); __i > 0; --__i) >> + { >> + _M_strides[__i - 1] = index_type(__stride); >> + __stride *= size_t(_M_extents.extent(__i - 1)); >> + } >> + } >> + >> + constexpr >> + mapping(const mapping&) noexcept = default; >> + >> + template<__mdspan::__valid_index_type<index_type> _OIndexType> >> + constexpr >> + mapping(const extents_type& __exts, >> + span<_OIndexType, extents_type::rank()> __strides) >> noexcept >> + : _M_extents(__exts) >> + { >> > We are not checking the preconditions here, could you add this information > to the commit > message, similarly as we have done for extents. > >> + for (size_t __i = 0; __i < extents_type::rank(); ++__i) >> + _M_strides[__i] = index_type(as_const(__strides[__i])); >> + } >> + >> + template<__mdspan::__valid_index_type<index_type> _OIndexType> >> + constexpr >> + mapping(const extents_type& __exts, >> + const array<_OIndexType, extents_type::rank()>& __strides) >> + noexcept >> + : mapping(__exts, >> + span<const _OIndexType, >> extents_type::rank()>(__strides)) >> + { } >> + >> + template<__mdspan::__mapping_like _StridedMapping> >> + requires (is_constructible_v<extents_type, >> + typename >> _StridedMapping::extents_type> >> + && _StridedMapping::is_always_unique() >> + && _StridedMapping::is_always_strided()) >> + constexpr explicit(!( >> + is_convertible_v<typename _StridedMapping::extents_type, >> extents_type> >> + && __mdspan::__standardized_mapping<_StridedMapping>)) >> + mapping(const _StridedMapping& __other) noexcept >> + : _M_extents(__other.extents()) >> + { >> + if constexpr (cmp_greater( >> + numeric_limits<typename >> _StridedMapping::index_type>::max(), >> + numeric_limits<index_type>::max())) >> + __glibcxx_assert(!cmp_less(numeric_limits<index_type>::max(), >> + __other.required_span_size())); >> > I would add messages to static assert, matching ones from extents. > As we made __offset efficient for standard layouts, I will add: > __glibcxx_assert(__mdspan::offset(_other) == 0); > >> + if constexpr (extents_type::rank() > 0) >> + for (size_t __i = 0; __i < extents_type::rank(); ++__i) >> + _M_strides[__i] = index_type(__other.stride(__i)); >> + } >> + >> + constexpr mapping& >> + operator=(const mapping&) noexcept = default; >> + >> + constexpr const _Extents& >> + extents() const noexcept { return _M_extents; } >> + >> + constexpr array<index_type, extents_type::rank()> >> + strides() const noexcept { >> + array<index_type, extents_type::rank()> __ret; >> + for (size_t __i = 0; __i < extents_type::rank(); ++__i) >> + __ret[__i] = _M_strides[__i]; >> + return __ret; >> + } >> + >> + constexpr index_type >> + required_span_size() const noexcept >> + { >> > We can use the fact that we have __static_exts and __dyn_exts and optimize > this to: > if constexpr (contains_zero(_static_exts<Mapping::extents_type>())) > return 0; > else if (contais_zero(_dyn_exts(_m.extents)) > return 0; > else > // Given this is 3rd time we are checking that (in __offset, > __respresetnable_size > > + for (size_t __i = 0; __i < extents_type::rank(); ++__i) >> + if (_M_extents.extent(__i) == 0) >> + return 0; >> + >> + index_type __ret = 1; >> + for (size_t __i = 0; __i < extents_type::rank(); ++__i) >> + __ret += (_M_extents.extent(__i) - 1) * _M_strides[__i]; >> + return __ret; >> + } >> + >> + template<__mdspan::__valid_index_type<index_type>... _Indices> >> + requires (sizeof...(_Indices) == extents_type::rank()) >> + constexpr index_type >> + operator()(_Indices... __indices) const noexcept >> + { >> + return __mdspan::__linear_index_strides(*this, >> + static_cast<index_type>(__indices)...); >> + } >> + >> + static constexpr bool >> + is_always_unique() noexcept { return true; } >> + >> + // _GLIBCXX_RESOLVE_LIB_DEFECTS >> + // 4266. layout_stride::mapping should treat empty mappings as >> exhaustive >> + static constexpr bool >> + is_always_exhaustive() noexcept >> + { >> > In the issue I have suggested moving the rank check here: > *> Returns*: true if *rank_* is 0 or if there is a rank index r of > extents() such that extents_type::static_extent(r) is 0, otherwise false. > So we need to have rank() == 0 || contains_zeror. > >> + return __mdspan::__contains_zero( >> + __mdspan::__static_extents<_Extents>()); >> + } >> + >> + static constexpr bool >> + is_always_strided() noexcept { return true; } >> + >> + static constexpr bool >> + is_unique() noexcept { return true; } >> + >> + // _GLIBCXX_RESOLVE_LIB_DEFECTS >> + // 4266. layout_stride::mapping should treat empty mappings as >> exhaustive >> > + constexpr bool >> + is_exhaustive() const noexcept >> + { >> + constexpr auto __rank = extents_type::rank(); >> + if constexpr (__rank == 0) >> > And here we can say if constexpr (is_always_exhaustive()). > I would write this as: > if constexpr (!is_always_exhaustive()) > { > auto __size = __mdspan::__fwd_prod(_M_extents, __rank); > if (__size > 0) > return __size == required_span_size() > } > return true; > >> + return true; >> + else >> + { >> + // Here we deviate from the standard. The standard requires >> that >> + // for some extents with size zero, is_exhaustive returns >> false. >> > Remove this comment, as you mentioned that you are implementing the 4266 > issue. > >> + auto __size = __mdspan::__fwd_prod(_M_extents, __rank); >> + if (__size == 0) >> + return true; >> + return __size == required_span_size(); >> + } >> + } >> + >> + static constexpr bool >> + is_strided() noexcept { return true; } >> + >> + constexpr index_type >> + stride(rank_type __r) const noexcept { return _M_strides[__r]; } >> + >> + template<__mdspan::__mapping_like _OMapping> >> + requires ((extents_type::rank() == >> _OMapping::extents_type::rank()) >> + && _OMapping::is_always_strided()) >> + friend constexpr bool >> + operator==(const mapping& __self, const _OMapping& __other) >> noexcept >> + { >> + if (__self.extents() != __other.extents()) >> + return false; >> + if constexpr (extents_type::rank() > 0) >> + for (size_t __i = 0; __i < extents_type::rank(); ++__i) >> + if (__self.stride(__i) != __other.stride(__i)) >> > I think we want to use cmp_equal here, to avoid warnings for > signed/unsigned comparison. > >> + return false; >> + return __mdspan::__offset(__other) == 0; >> + } >> + >> + private: >> + using _S_strides_t = typename __array_traits<index_type, >> + >> extents_type::rank()>::_Type; >> + [[no_unique_address]] _Extents _M_extents{}; >> > We do not need brace {} here. > >> + [[no_unique_address]] _S_strides_t _M_strides; >> > > >> + }; >> + >> _GLIBCXX_END_NAMESPACE_VERSION >> } >> #endif >> diff --git a/libstdc++-v3/src/c++23/std.cc.in b/libstdc++-v3/src/c++23/ >> std.cc.in >> index e9b5709203c..edcfaa397c1 100644 >> --- a/libstdc++-v3/src/c++23/std.cc.in >> +++ b/libstdc++-v3/src/c++23/std.cc.in >> @@ -1843,7 +1843,8 @@ export namespace std >> using std::extents; >> using std::layout_left; >> using std::layout_right; >> - // FIXME layout_*, default_accessor and mdspan >> + using std::layout_stride; >> + // FIXME layout_left_padded, layout_right_padded, default_accessor and >> mdspan >> } >> #endif >> >> -- >> 2.49.0 >> >>
