Hi, As we will be landing patches for extends, this will become a separate patch series. I would prefer, if you could commit per layout, and start with layout_right (default) I try to provide prompt responses, so if that works better for you, you can post a patch only with this layout first, as most of the comments will apply to all of them.
For the general design we have constructors that allow conversion between rank-0 and rank-1 layouts left and right. This is done because they essentially represents the same layout. I think we could benefit from that in code by having a base classes for rank0 and rank1 mapping: template<typename _Extents> _Rank0_mapping_base { static_assert(_Extents::rank() == 0); template<OtherExtents> // explicit, requires goes here _Rank0_mapping_base(_Rank0_mapping_base<OtherExtents>); // All members layout_type goes her }; template<typename _Extents> _Rank1_mapping_base { static_assert(_Extents::rank() == 1); // Static assert for product is much simpler here, as we need to check one template<OtherExtents> // explicit, requires goes here _Rank1_mapping_base(_Rank1_mapping_base<OtherExtents>); // Call operator can also be simplified index_type operator()(index_type i) const // conversion happens at user side // cosntructor from strided_layout of Rank1 goes here. // All members layout_type goes her }; Then we will specialize layout_left/right/stride to use _Rank0_mapping_base as a base for rank() == 0 and layout_left/right to use _Rank1_mapping as base for rank()1; template<typename T, unsigned... Ids> struct extents {}; struct layout { template<typename Extends> struct mapping { // static assert that Extents mmyst be specialization of _Extents goes here. } }; template<typename _IndexType> struct layout::mapping<extents<_IndexType>> : _Rank0_mapping_base<extents<_IndexType>> { using layout_type = layout_left; // Provides converting constructor. using _Rank0_mapping_base<extents<_IndexType>>::_Rank0_mapping_base; // This one is implicit; mapping(_Rank0_mapping_base<extents<_IndexType>> const&); }; template<typename _IndexType, unsigned _Ext> struct layout::mapping<extents<_IndexType, _Ext>> : _Rank1_mapping_base<extents<_IndexType>> { using layout_type = layout_left; // Provides converting constructor. using _Rank0_mapping_base<extents<_IndexType>>::_Rank0_mapping_base; // This one is implicit, allows construction from layout_right mapping(_Rank1_mapping_base<extents<_IndexType>> const&); }; }; template<typename _IndexType, unsigned... _Ext> requires sizeof..(_Ext) > = 2 struct layout::mapping<extents<_IndexType, _Ext>> The last one is a generic implementation that you can use in yours. Please also include a comment explaining that we are deviating from standard text here. On Tue, Apr 29, 2025 at 2:56 PM Luc Grosheintz <luc.groshei...@gmail.com> wrote: > Implements the parts of layout_left that don't depend on any of the > other layouts. > > libstdc++/ChangeLog: > > * include/std/mdspan (layout_left): New class. > > Signed-off-by: Luc Grosheintz <luc.groshei...@gmail.com> > --- > libstdc++-v3/include/std/mdspan | 179 ++++++++++++++++++++++++++++++++ > 1 file changed, 179 insertions(+) > > diff --git a/libstdc++-v3/include/std/mdspan > b/libstdc++-v3/include/std/mdspan > index 39ced1d6301..e05048a5b93 100644 > --- a/libstdc++-v3/include/std/mdspan > +++ b/libstdc++-v3/include/std/mdspan > @@ -286,6 +286,26 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > namespace __mdspan > { > + template<typename _Extents> > + constexpr typename _Extents::index_type > + __fwd_prod(const _Extents& __exts, size_t __r) noexcept > + { > + typename _Extents::index_type __fwd = 1; > + for(size_t __i = 0; __i < __r; ++__i) > + __fwd *= __exts.extent(__i); > + return __fwd; > + } > As we are inside the standard library implementation, we can do some tricks here, and provide two functions: // Returns the std::span(_ExtentsStorage::_Ext).substr(f, l); // For extents forward to __static_exts span<typename Extends::index_type> __static_exts(size_t f, size_t l); // Returns the std::span(_ExtentsStorage::_M_dynamic_extents).substr(_S_dynamic_index[f], _S_dynamic_index[l); span<typename Extends::index_type> __dynamic_exts(Extents const& c); Then you can befriend this function both to extents and _ExtentsStorage. Also add index_type members to _ExtentsStorage. Then instead of having fwd-prod and rev-prod I would have: template<typename _Extents> consteval size_t __static_ext_prod(size_t f, size_t l) { // multiply E != dynamic_ext from __static_exts } constexpr size __ext_prod(const _Extents& __exts, size_t f, size_t l) { // multiply __static_ext_prod<_Extents>(f, l) and each elements of __dynamic_exts(__exts, f, l); } Then fwd-prod(e, n) would be __ext_prod(e, 0, n), and rev_prod(e, n) would be __ext_prod(e, __ext.rank() -n, n, __ext.rank()) > + > + template<typename _Extents> > + constexpr typename _Extents::index_type > + __rev_prod(const _Extents& __exts, size_t __r) noexcept > + { > + typename _Extents::index_type __rev = 1; > + for(size_t __i = __r + 1; __i < __exts.rank(); ++__i) > + __rev *= __exts.extent(__i); > + return __rev; > + } > + > template<typename _IndexType, size_t... _Counts> > auto __build_dextents_type(integer_sequence<size_t, _Counts...>) > -> extents<_IndexType, ((void) _Counts, dynamic_extent)...>; > @@ -304,6 +324,165 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > explicit extents(_Integrals...) -> > extents<size_t, __mdspan::__dynamic_extent<_Integrals>()...>; > > + struct layout_left > + { > + template<typename _Extents> > + class mapping; > + }; > + > + namespace __mdspan > + { > + template<typename _Tp> > + constexpr bool __is_extents = false; > + > + template<typename _IndexType, size_t... _Extents> > + constexpr bool __is_extents<extents<_IndexType, _Extents...>> = > true; > + > + template<size_t _Count> > + struct _LinearIndexLeft > + { > + template<typename _Extents, typename... _Indices> > + static constexpr typename _Extents::index_type > + _S_value(const _Extents& __exts, typename _Extents::index_type > __idx, > + _Indices... __indices) noexcept > + { > + return __idx + __exts.extent(_Count) > + * _LinearIndexLeft<_Count + 1>::_S_value(__exts, __indices...); > + } > + > + template<typename _Extents> > + static constexpr typename _Extents::index_type > + _S_value(const _Extents&) noexcept > + { return 0; } > + }; > + > + template<typename _Extents, typename... _Indices> > + constexpr typename _Extents::index_type > + __linear_index_left(const _Extents& __exts, _Indices... __indices) > + { > + return _LinearIndexLeft<0>::_S_value(__exts, __indices...); > + } > This can be eliminated by fold expressions, see below. > + > + template<typename _IndexType, typename _Tp, size_t _Nm> > + consteval bool > + __is_representable_product(array<_Tp, _Nm> __factors) > + { > + size_t __rest = numeric_limits<_IndexType>::max(); > + for(size_t __i = 0; __i < _Nm; ++__i) > + { > + if (__factors[__i] == 0) > + return true; > + __rest /= _IndexType(__factors[__i]); > + } > + return __rest > 0; > + } > I would replace that with template<IndexType> consteval size_t __div_reminder(span<const size_t, _Nm> __factors, size_t __val) { size_t __rest = val; for(size_t __i = 0; __i < _Nm; ++__i) { if (__factors[__i] == dynamic_extent) continue; if (__factors[__i] != 0) return val; __rest /= _IndexType(__factors[__i]); if (__res == 0) return 0; } return __rest; } We can express the is presentable check as static constexpr __dyn_reminder = __div_reminder(__static_exts<Extents>(0, rank()), std::numeric_limits<Index>::max()); static_assert(__dyn_reminder > 0); However, with __dyn_reminder value, the precondition https://eel.is/c++draft/mdspan.layout#left.cons-1, can be checked by doing equivalent of __div_remainder for __dyn_extents with __val being __dyn_reminder. > + > + template<typename _Extents> > + consteval array<typename _Extents::index_type, _Extents::rank()> > + __static_extents_array() > + { > + array<typename _Extents::index_type, _Extents::rank()> __exts; > + for(size_t __i = 0; __i < _Extents::rank(); ++__i) > + __exts[__i] = _Extents::static_extent(__i); > + return __exts; > + } > Replaced by __static_exts accessor, as described above. > + > + template<typename _Extents, typename _IndexType> > + concept __representable_size = _Extents::rank_dynamic() != 0 > + || __is_representable_product<_IndexType>( > + __static_extents_array<_Extents>()); > + > + template<typename _Extents> > + concept __layout_extent = __representable_size< > + _Extents, typename _Extents::index_type>; > + } > + > + template<typename _Extents> > + class layout_left::mapping > + { > + static_assert(__mdspan::__layout_extent<_Extents>, > + "The size of extents_type is not representable as index_type."); > + 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_left; > + > + constexpr > + mapping() noexcept = default; > + > + constexpr > + mapping(const mapping&) noexcept = default; > + > + constexpr > + mapping(const extents_type& __extents) noexcept > + : _M_extents(__extents) > + { > > } > + > + template<typename _OExtents> > + requires (is_constructible_v<extents_type, _OExtents>) > + constexpr explicit(!is_convertible_v<_OExtents, extents_type>) > + mapping(const mapping<_OExtents>& __other) noexcept > + : _M_extents(__other.extents()) > + { Here we could do checks at compile time: if constexpr(_OExtents::rank_dynamic() == 0) static_assert( __div_remainder(...) > 0); } > } > + > + constexpr mapping& > + operator=(const mapping&) noexcept = default; > + > + constexpr const extents_type& > + extents() const noexcept { return _M_extents; } > + > + constexpr index_type > + required_span_size() const noexcept > + { return __mdspan::__fwd_prod(_M_extents, _M_extents.rank()); } > + > + template<__mdspan::__valid_index_type<index_type>... _Indices> > // Because we extracted rank0 and rank1 specializations > + requires (sizeof...(_Indices) + 1 == extents_type::rank()) > + constexpr index_type > + operator()(index_type __idx, _Indices... __indices) const noexcept > + { > This could be implemented as, please synchronize the names. if constexpr (!is_same_v<_Indices, index_type> || ...) // Reduce the number of instantations. return operator()(index_type _idx0, static_cast<index_type>(std::move(__indices))....); else { // This can be used for layout stride, if you start with __res = 0; index_type __res = _idx0; index_type __mult = _M_extents.extent(0); auto __update = [&__res, &__mult, __pos = 1u](index_type __idx) mutable { __res += __idx * __mult; __mult *= _M_extents.extent(__pos); ++__pos; }; // Fold over invocation of lambda (__update(_Indices), ....); return __res; } This could be even simpler and written as (use for layout stride): size_t __pos = 0; return (index_type(0) + ... + __indices * stride(__pos++)); Here, I prefer to avoid multiplying multiple times. + return __mdspan::__linear_index_left( > + _M_extents, static_cast<index_type>(__indices)...); > + } > + > + static constexpr bool > + is_always_unique() noexcept { return true; } > + > + static constexpr bool > + is_always_exhaustive() noexcept { return true; } > + > + static constexpr bool > + is_always_strided() noexcept { return true; } > + > + static constexpr bool > + is_unique() noexcept { return true; } > + > + static constexpr bool > + is_exhaustive() noexcept { return true; } > + > + static constexpr bool > + is_strided() noexcept { return true; } > + > + constexpr index_type > + stride(rank_type __i) const noexcept > + requires (extents_type::rank() > 0) > + { > + __glibcxx_assert(__i < extents_type::rank()); > + return __mdspan::__fwd_prod(_M_extents, __i); > + } > + > + template<typename _OExtents> > + requires (extents_type::rank() == _OExtents::rank()) > + friend constexpr bool > + operator==(const mapping& __self, const mapping<_OExtents>& > __other) > + noexcept > + { return __self.extents() == __other.extents(); } > + > + private: > + [[no_unique_address]] extents_type _M_extents; > + }; > + > _GLIBCXX_END_NAMESPACE_VERSION > } > #endif > -- > 2.49.0 > >