On Fri, May 30, 2025 at 12:01 PM Luc Grosheintz <luc.groshei...@gmail.com> wrote:
> > > On 5/29/25 10:07, Tomasz Kaminski wrote: > > 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. > > > > > > I'm not sure I'm following. There's no static_assert nearby. Would you like > me to add > > __glibcxx_assert(!cmd_less(...) && "other.required_span_size() must be" > " representable as index_type"); > Yes, something like that. So the test can look for "must be representable as". > > > >> 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 > >>> > >>> > > > >
