On Tue, May 20, 2025 at 4:30 PM Luc Grosheintz <luc.groshei...@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.groshei...@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.groshei...@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.
>
> 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.
>
> >
> > 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
> >>
> >>
> >
>
>
