[PATCH v4 6/8] libstdc++: Implement layout_stride from mdspan.
Luc Grosheintz
luc.grosheintz@gmail.com
Fri May 30 10:01:15 GMT 2025
On 5/29/25 10:07, Tomasz Kaminski wrote:
> 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.
>
>
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");
>> 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
>>>
>>>
>
More information about the Libstdc++
mailing list