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())); > 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 > >
