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");


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




Reply via email to