[PATCH v4 6/8] libstdc++: Implement layout_stride from mdspan.
Tomasz Kaminski
tkaminsk@redhat.com
Thu May 29 08:07:14 GMT 2025
On Thu, May 29, 2025 at 9:23 AM Tomasz Kaminski <tkaminsk@redhat.com> wrote:
>
>
>
> On Mon, May 26, 2025 at 4:13 PM Luc Grosheintz <luc.grosheintz@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.grosheintz@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
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20250529/67134538/attachment-0001.htm>
More information about the Libstdc++
mailing list