[PATCH v2 1/6] libstdc++: Implement layout_left from mdspan.
Luc Grosheintz
luc.grosheintz@gmail.com
Tue May 20 19:08:09 GMT 2025
On 5/20/25 17:44, Tomasz Kaminski wrote:
> On Tue, May 20, 2025 at 4:30 PM Luc Grosheintz <luc.grosheintz@gmail.com>
> wrote:
>
>>
>>
>> On 5/20/25 4:07 PM, Tomasz Kaminski wrote:
>>> On Tue, May 20, 2025 at 3:16 PM Luc Grosheintz <luc.grosheintz@gmail.com
>>>
>>> wrote:
>>>
>>>> Implements the parts of layout_left that don't depend on any of the
>>>> other layouts.
>>>>
>>>> libstdc++-v3/ChangeLog:
>>>>
>>>> * include/std/mdspan (layout_left): New class.
>>>>
>>>> Signed-off-by: Luc Grosheintz <luc.grosheintz@gmail.com>
>>>> ---
>>>
>>> Sending feedback on this PR, I do not think I will have time to review
>> the
>>> remaining ones today.
>>> Thanks, for the update, this really looks good. Good job on catching
>>> rank_dynamic==0 case.
>>>
>>> I have added more stylistic suggestions:
>>> I would consider renaming the internal function to `exts` instead of
>>> `subextents`/`extents`.
>>> We are inside __mdspan namespace, so risk of collision is minimal. Also
>>> added few suggestion
>>> for default arguments.
I agree, this is clumsy. I like static_extents and dynamic_extents.
They feel appropriately descriptive and have a reasonable length. The
problem is that the member _M_dynamic_extents already existed.
To fix this we could rename the member: `_M_dyn_exts` and the accessor
`_M_dynamic_extents`.
Similary, we'd rename extents::_M_dynamic_extents to `_M_exts` because
that member doesn't refer to exclusively dynamic extents. I wanted to
do this.
In doing so we keep names with short "range" short and names with longer
"range" longer.
This way the names `__sta_exts` and `__dyn_exts` are still available
for local names.
After doing this and with the default arguments, everything is still
short enough to be one-liners.
Let me know if you'd still prefer reducing the names as you propose.
>>
>> Many thanks for the review and no rush! One clarifying question, since
>> some of the loops have (or will have) short-circuiting, do you mean
>> std::for_each or range-based for-loops `for(auto __factor : __exts)`. Do
>> you mind if I use the latter?
>>
> I mean range-based for-loops. Do not need to go for for_each.
Yes, thanks for noticing.
>
>>
>>>
>>> More comments below.
>>>
>>>> libstdc++-v3/include/std/mdspan | 309 +++++++++++++++++++++++++++++++-
>>>> 1 file changed, 308 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/libstdc++-v3/include/std/mdspan
>>>> b/libstdc++-v3/include/std/mdspan
>>>> index 47cfa405e44..d90fed57a19 100644
>>>> --- a/libstdc++-v3/include/std/mdspan
>>>> +++ b/libstdc++-v3/include/std/mdspan
>>>> @@ -144,6 +144,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>> { return __exts[__i]; });
>>>> }
>>>>
>>>> + static constexpr span<const size_t>
>>>> + _S_static_subextents(size_t __begin, size_t __end) noexcept
>>>>
>>> + {
>>>> + return {_Extents.data() + __begin, _Extents.data() + __end};
>>>> + }
>>>> +
>>>> + constexpr span<const _IndexType>
>>>> + _M_dynamic_subextents(size_t __begin, size_t __end) const
>> noexcept
>>>> + requires (_Extents.size() > 0)
>>>> + {
>>>> + return {_M_dynamic_extents + _S_dynamic_index[__begin],
>>>> + _M_dynamic_extents + _S_dynamic_index[__end]};
>>>> + }
>>>> +
>>>> private:
>>>> using _S_storage = __array_traits<_IndexType,
>>>> _S_rank_dynamic>::_Type;
>>>> [[no_unique_address]] _S_storage _M_dynamic_extents;
>>>> @@ -160,6 +174,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>> || _Extent <= numeric_limits<_IndexType>::max();
>>>> }
>>>>
>>>> + namespace __mdspan
>>>> + {
>>>> + template<typename _Extents>
>>>> + constexpr span<const size_t>
>>>> + __static_subextents(size_t __begin, size_t __end)
>>>>
>>> Consider adding default arguments: __begin = 0u, __end =
>> _Extents::rank().
>>> This would simplify calls.
>>>
>>>> + { return _Extents::_S_storage::_S_static_subextents(__begin,
>>>> __end); }
>>>> +
>>>> + template<typename _Extents>
>>>> + constexpr span<const typename _Extents::index_type>
>>>> + __dynamic_subextents(const _Extents& __exts, size_t __begin,
>> size_t
>>>> __end)
>>>> + {
>>>> + return __exts._M_dynamic_extents._M_dynamic_subextents(__begin,
>>>> __end);
>>>> + }
>>>> + }
>>>> +
>>>> template<typename _IndexType, size_t... _Extents>
>>>> class extents
>>>> {
>>>> @@ -251,7 +280,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>> : _M_dynamic_extents(span<const _OIndexType, _Nm>(__exts))
>>>> { }
>>>>
>>>> -
>>>> template<__mdspan::__valid_index_type<index_type> _OIndexType,
>>>> size_t _Nm>
>>>> requires (_Nm == rank() || _Nm == rank_dynamic())
>>>> constexpr explicit(_Nm != rank_dynamic())
>>>> @@ -276,6 +304,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>> }
>>>>
>>>> private:
>>>> + friend span<const size_t>
>>>> + __mdspan::__static_subextents<extents>(size_t, size_t);
>>>> +
>>>> + friend span<const index_type>
>>>> + __mdspan::__dynamic_subextents<extents>(const extents&, size_t,
>>>> size_t);
>>>> +
>>>> using _S_storage = __mdspan::_ExtentsStorage<
>>>> _IndexType, array<size_t, sizeof...(_Extents)>{_Extents...}>;
>>>> [[no_unique_address]] _S_storage _M_dynamic_extents;
>>>> @@ -286,6 +320,52 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>>
>>>> namespace __mdspan
>>>> {
>>>> + template<typename _Extents>
>>>> + constexpr size_t
>>>> + __static_extents_prod(size_t __begin, size_t __end)
>>>> + {
>>>>
>>> + auto __sta_exts = __static_subextents<_Extents>(__begin, __end);
>>>> + size_t __ret = 1;
>>>> + for(size_t __i = 0; __i < __sta_exts.size(); ++__i)
>>>>
>>> Again we could use each here, once we have span.
>>>
>>>> + if (__sta_exts[__i] != dynamic_extent)
>>>> + __ret *= __sta_exts[__i];
>>>> + return __ret;
>>>> + }
>>>> +
>>>> + template<typename _Extents>
>>>> + constexpr size_t
>>>> + __dynamic_extents_prod(const _Extents& __exts, size_t __begin,
>>>> + size_t __end)
>>>> + {
>>>> + auto __dyn_exts = __dynamic_subextents<_Extents>(__exts,
>> __begin,
>>>> + __end);
>>>> + size_t __ret = 1;
>>>> + for(size_t __i = 0; __i < __dyn_exts.size(); ++__i)
>>>>
>>> Again we could use each here, once we have span. And we could inline the
>>> function in __exts_prod.
>>>
>>>> + __ret *= __dyn_exts[__i];
>>>> + return __ret;
>>>> + }
>>>> +
>>>> + template<typename _Extents>
>>>> + constexpr typename _Extents::index_type
>>>> + __exts_prod(const _Extents& __exts, size_t __begin, size_t __end)
>>>> noexcept
>>>> + {
>>>> + using _IndexType = typename _Extents::index_type;
>>>> + auto __ret = __static_extents_prod<_Extents>(__begin, __end);
>>>> + if constexpr (_Extents::rank_dynamic() > 0)
>>>> + __ret *= __dynamic_extents_prod(__exts, __begin, __end);
>>>> + return __ret;
>>>> + }
>>>> +
>>>> + template<typename _Extents>
>>>> + constexpr typename _Extents::index_type
>>>> + __fwd_prod(const _Extents& __exts, size_t __r) noexcept
>>>> + { return __exts_prod(__exts, 0, __r); }
>>>> +
>>>> + template<typename _Extents>
>>>> + constexpr typename _Extents::index_type
>>>> + __rev_prod(const _Extents& __exts, size_t __r) noexcept
>>>> + { return __exts_prod(__exts, __r + 1, __exts.rank()); }
>>>> +
>>>> template<typename _IndexType, size_t... _Counts>
>>>> auto __build_dextents_type(integer_sequence<size_t, _Counts...>)
>>>> -> extents<_IndexType, ((void) _Counts, dynamic_extent)...>;
>>>> @@ -304,6 +384,233 @@ _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<typename _Extents, typename... _Indices>
>>>> + constexpr typename _Extents::index_type
>>>> + __linear_index_left(const _Extents& __exts, _Indices...
>> __indices)
>>>> + {
>>>> + using _IndexType = typename _Extents::index_type;
>>>> + _IndexType __res = 0;
>>>> + if constexpr (sizeof...(__indices) > 0)
>>>> + {
>>>> + _IndexType __mult = 1;
>>>> + auto __update = [&, __pos = 0u](_IndexType __idx) mutable
>>>> + {
>>>> + __res += __idx * __mult;
>>>> + __mult *= __exts.extent(__pos);
>>>> + ++__pos;
>>>> + };
>>>> + (__update(__indices), ...);
>>>> + }
>>>> + return __res;
>>>> + }
>>>> +
>>>> + template<typename _Tp>
>>>> + constexpr bool
>>>> + __contains_zero(span<_Tp> __exts)
>>>> + {
>>>> + for (size_t __i = 0; __i < __exts.size(); ++__i)
>>>> + if (__exts[__i] == 0)
>>>> + return true;
>>>> + return false;
>>>> + }
>>>> +
>>>> + template<typename _Extents>
>>>> + consteval bool
>>>> + __has_static_zero()
>>>>
>>> With default arguments this will become
>>> __contains_zero(_static_exts<_Extent>()).
>>> I would inline it then.
>>>
>>>> + {
>>>> + return __contains_zero(
>>>> + __static_subextents<_Extents>(0, _Extents::rank()));
>>>> + }
>>>> +
>>>> + template<typename _Extents, typename _IndexType>
>>>> + consteval _IndexType
>>>> + __static_quotient(_IndexType __nom)
>>>>
>>> Consider adding the default argument for numeric_limits max here.
>>>
>>>> + {
>>>> + for(size_t __i = 0; __i < _Extents::rank(); ++__i)
>>>>
>>> Use foreach on static_exts here.
>>>
>>>> + {
>>>>
>>> I would check if (__norm) is zero with early exit.
>>>
>>>> + auto __factor = _Extents::static_extent(__i);
>>>> + if (__factor != dynamic_extent)
>>>> + __nom /= _IndexType(__factor);
>>>> + }
>>>> + return __nom;
>>>> + }
>>>> +
>>>> + template<typename _Extents>
>>>> + constexpr bool
>>>> + __is_representable_extents(const _Extents& __exts)
>>>> + {
>>>> + using _IndexType = _Extents::index_type;
>>>> + constexpr auto __rank = _Extents::rank();
>>>> +
>>>> + constexpr auto __sta_quo = __static_quotient<_Extents,
>> _IndexType>(
>>>> + numeric_limits<_IndexType>::max());
>>>>
>>> You are calling static_quotient here before checking if we have zero in
>>> static asserts.
>>> This will lead to division by zero. Consider using if with initializer,
>> or
>>> at least nested scope.
>>> if constepxrt (static-zero)
>>> else if constexpr(constexpr auto __static_quotient<_Extents,
>>> _IndexType>());
>>> _Extents::rank_dynamic() == 0)
>>> return __static_quotient != 0;
>>> else if (auto __dyn_exts = __dynamic_subextents(__exts);
>>> __contains_zero(__dyn_exts))
>>> return true;
>>> else if constexpr(__sta_quo == 0)
>>> return false;
>>> else
>>> {
>>> }
>>>
>>>> +
>>>> + if constexpr (__has_static_zero<_Extents>())
>>>> + return true;
>>>> + else if constexpr (_Extents::rank_dynamic() == 0)
>>>>
>>> We have already checked this case in mandates. But I think it's worth
>>> having it here again.
>>>
>>>> + return __sta_quo != 0;
>>>> + else
>>>> + {
>>>> + auto __dyn_exts = __dynamic_subextents(__exts, 0, __rank);
>>>> + if(__contains_zero(__dyn_exts))
>>>> + return true;
>>>> +
>>>> + if constexpr (__sta_quo == 0)
>>>> + return false;
>>>> + else
>>>> + {
>>>> + constexpr auto __dyn_rank = _Extents::rank_dynamic();
>>>> + auto __dyn_quo = __sta_quo;
>>>> + for(size_t __i = 0; __i < __dyn_rank && __dyn_quo > 0;
>> ++__i)
>>>>
>>> Would prefer if you will use for_each here, with nested if __dyn_quo ==
>> 0.
>>>
>>>> + __dyn_quo /= __dyn_exts[__i];
>>>> + return __dyn_quo != 0;
>>>> + }
>>>> + }
>>>> + }
>>>> +
>>>> + template<typename _Extents, typename _IndexType>
>>>> + concept __representable_size = _Extents::rank_dynamic() != 0
>>>> + || __has_static_zero<_Extents>()
>>>> + || (__static_quotient<_Extents, _IndexType>(
>>>> + numeric_limits<_IndexType>::max()) != 0);
>>>> +
>>>> + template<typename _Layout, typename _Mapping>
>>>> + concept __mapping_of =
>>>> + is_same_v<typename _Layout::mapping<typename
>>>> _Mapping::extents_type>,
>>>> + _Mapping>;
>>>> +
>>>> + template<typename _Mapping>
>>>> + concept __standardized_mapping = __mapping_of<layout_left,
>>>> _Mapping>;
>>>> +
>>>> + template<typename M>
>>>> + concept __mapping_like = requires
>>>> + {
>>>> + requires __is_extents<typename M::extents_type>;
>>>> + { M::is_always_strided() } -> same_as<bool>;
>>>> + { M::is_always_exhaustive() } -> same_as<bool>;
>>>> + { M::is_always_unique() } -> same_as<bool>;
>>>> + bool_constant<M::is_always_strided()>::value;
>>>> + bool_constant<M::is_always_exhaustive()>::value;
>>>> + bool_constant<M::is_always_unique()>::value;
>>>> + };
>>>> +
>>>> + // A tag type to create internal ctors.
>>>> + class __internal_ctor
>>>> + { };
>>>> + }
>>>> +
>>>> + template<typename _Extents>
>>>> + class layout_left::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_left;
>>>> +
>>>> + static_assert(__mdspan::__representable_size<_Extents,
>> index_type>,
>>>> + "The size of extents_type must be representable as index_type");
>>>> +
>>>> + constexpr
>>>> + mapping() noexcept = default;
>>>> +
>>>> + constexpr
>>>> + mapping(const mapping&) noexcept = default;
>>>> +
>>>> + constexpr
>>>> + mapping(const _Extents& __extents) noexcept
>>>> + : _M_extents(__extents)
>>>> + {
>>>> __glibcxx_assert(__mdspan::__is_representable_extents(_M_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
>>>> + : mapping(__other.extents(), __mdspan::__internal_ctor{})
>>>> + { }
>>>> +
>>>> + constexpr mapping&
>>>> + operator=(const mapping&) noexcept = default;
>>>> +
>>>> + constexpr const _Extents&
>>>> + extents() const noexcept { return _M_extents; }
>>>> +
>>>> + constexpr index_type
>>>> + required_span_size() const noexcept
>>>> + { return __mdspan::__fwd_prod(_M_extents, extents_type::rank());
>> }
>>>> +
>>>> + 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_left(
>>>> + this->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:
>>>> + template<typename _OExtents>
>>>> + constexpr explicit
>>>> + mapping(const _OExtents& __oexts, __mdspan::__internal_ctor)
>>>> noexcept
>>>> + : _M_extents(extents_type(__oexts))
>>>> + {
>>>> + static_assert(__mdspan::__representable_size<_OExtents,
>>>> index_type>,
>>>> + "The size of OtherExtents must be representable as
>>>> index_type");
>>>> +
>>>> __glibcxx_assert(__mdspan::__is_representable_extents(_M_extents));
>>>> + }
>>>> +
>>>> + public:
>>>> + [[no_unique_address]] _Extents _M_extents;
>>>> + };
>>>> +
>>>> _GLIBCXX_END_NAMESPACE_VERSION
>>>> }
>>>> #endif
>>>> --
>>>> 2.49.0
>>>>
>>>>
>>>
>>
>>
>
More information about the Libstdc++
mailing list