On Mon, Jul 28, 2025 at 10:50 AM Luc Grosheintz <luc.groshei...@gmail.com>
wrote:
>
>
> On 7/28/25 08:13, Tomasz Kaminski wrote:
> > On Sun, Jul 27, 2025 at 2:57 PM Luc Grosheintz <luc.groshei...@gmail.com
> >
> > wrote:
> >
> >> In mdspan related code involving static extents, often the IndexType is
> >> part of the template parameters, even though it's not needed.
> >>
> >> This commit extracts the parts of _ExtentsStorage not related to
> >> IndexType into a separate class _StaticExtents.
> >>
> >> It also prefers passing the array of static extents, instead of the
> >> whole extents object where possible.
> >>
> >> These changes don't effect the compilation time, i.e. the difference was
> >> (much) less than 10%. However, the size of the object file is noticeably
> >> impacted.
> >>
> >> Consider an extreme example that computes required_span_size and stride
> >> for all combinations of:
> >>
> >> - all three layouts,
> >> - three choices of static extents Indices...
> >> - eight choices of IndexType.
> >>
> >> In this case the size of the object file reduced by 45% (22.3kB ->
> >> 12.1kB) when compiling with -O2. Using objdump -h we see that the size
> >> of .text is unchanged, but .rodata decreases. This is plausible,
> >> because:
> >>
> >> - the deduplicated code is short and is always inlined,
> >> - the static storage for _FwdProd, _RevProd, the NTTP _Extents and
> >> _StaticExtents::_S_dynamic_index is not deduplicated before this
> >> commit.
> >>
> >> libstdc++-v3/ChangeLog:
> >>
> >> * include/std/mdspan (__mdspan::_StaticExtents): New class.
> >> (__mdspan::_ExtentsStorage): Inherit non-IndexType related
> >> methods from _StaticExtents.
> >> (__mdspan::_ExtentsStorage::_S_static_extents): Return
> reference
> >> to NTTP _Extents.
> >> (__mdspan::__static_extents): Ditto.
> >> (__mdspan::__static_prod): Use NTTP for static extents.
> >> (__mdspan::_FwdProd): Ditto.
> >> (__mdspan::_RevProd): Ditto.
> >> (__mdspan::__contains_zero): New overload for const array&.
> >>
> >> Signed-off-by: Luc Grosheintz <luc.groshei...@gmail.com>
> >> ---
> >>
> > I have given you the same suggestion while reviewing my previous commit,
> > so this looks good to me. Would it be much of the issue to put this
> comment
> > before the changes introducing the caching of indices when doing new
> > revision.
> > This is not strictly necessary, but this ones is not tradeoff between
> conde
> > size and
> > performance.
>
> I'd prefer not to, the diffs don't commute and require some temporary minor
> inconvenience to eliminate one case of __static_extents(__begin, __end).
>
As I said, I do not have a strong preference in either direction. So, you
can keep the
current order.
>
> Maybe a better excuse: this way around we can focus on code in .text first
> and then check that .rodata and object file size shrink down to the
> expected
> size. Somehow, this order of the git messages feels more interesting than
> the
> reverse. In the reverse we first check object size; then completely rewrite
> how we use static variables; making the previous commit message instantly
> outdated.
>
> >
> > libstdc++-v3/include/std/mdspan | 70 +++++++++++++++++++++------------
> >> 1 file changed, 45 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/libstdc++-v3/include/std/mdspan
> >> b/libstdc++-v3/include/std/mdspan
> >> index 5b47c95d2c6..2cf572f1410 100644
> >> --- a/libstdc++-v3/include/std/mdspan
> >> +++ b/libstdc++-v3/include/std/mdspan
> >> @@ -49,19 +49,14 @@ namespace std _GLIBCXX_VISIBILITY(default)
> >> _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> namespace __mdspan
> >> {
> >> - template<typename _IndexType, array _Extents>
> >> - class _ExtentsStorage
> >> + template<array _Extents>
> >> + class _StaticExtents
> >> {
> >> public:
> >> static consteval bool
> >> _S_is_dyn(size_t __ext) noexcept
> >> { return __ext == dynamic_extent; }
> >>
> >> - template<typename _OIndexType>
> >> - static constexpr _IndexType
> >> - _S_int_cast(const _OIndexType& __other) noexcept
> >> - { return _IndexType(__other); }
> >> -
> >> static constexpr size_t _S_rank = _Extents.size();
> >>
> >> // For __r in [0, _S_rank], _S_dynamic_index[__r] is the number
> >> @@ -99,6 +94,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> static constexpr size_t
> >> _S_static_extent(size_t __r) noexcept
> >> { return _Extents[__r]; }
> >> + };
> >> +
> >> + template<typename _IndexType, array _Extents>
> >> + class _ExtentsStorage : public _StaticExtents<_Extents>
> >> + {
> >> + private:
> >> + using _S_base = _StaticExtents<_Extents>;
> >> +
> >> + public:
> >> + using _S_base::_S_rank;
> >> + using _S_base::_S_rank_dynamic;
> >> + using _S_base::_S_dynamic_index;
> >> + using _S_base::_S_dynamic_index_inv;
> >> +
> >> + template<typename _OIndexType>
> >> + static constexpr _IndexType
> >> + _S_int_cast(const _OIndexType& __other) noexcept
> >> + { return _IndexType(__other); }
> >>
> >> constexpr _IndexType
> >> _M_extent(size_t __r) const noexcept
> >> @@ -157,9 +170,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> { return __exts[__i]; });
> >> }
> >>
> >> - static constexpr span<const size_t>
> >> - _S_static_extents(size_t __begin, size_t __end) noexcept
> >> - { return {_Extents.data() + __begin, _Extents.data() + __end}; }
> >> + static constexpr const array<size_t, _S_rank>&
> >> + _S_static_extents() noexcept
> >> + { return _Extents; }
> >>
> >> constexpr span<const _IndexType>
> >> _M_dynamic_extents(size_t __begin, size_t __end) const noexcept
> >> @@ -184,27 +197,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> __valid_static_extent = _Extent == dynamic_extent
> >> || _Extent <= numeric_limits<_IndexType>::max();
> >>
> >> - template<typename _Extents>
> >> + template<array _Extents>
> >> consteval size_t
> >> __static_prod(size_t __begin, size_t __end)
> >> {
> >> size_t __prod = 1;
> >> for(size_t __i = __begin; __i < __end; ++__i)
> >> {
> >> - auto __ext = _Extents::static_extent(__i);
> >> + auto __ext = _Extents[__i];
> >> __prod *= __ext == dynamic_extent ? size_t(1) : __ext;
> >> }
> >> return __prod;
> >> }
> >>
> >> // Pre-compute: \prod_{i = 0}^r _Extents[i]
> >> - template<typename _Extents>
> >> + template<array _Extents>
> >> struct _FwdProd
> >> {
> >> - constexpr static std::array<size_t, _Extents::rank() + 1>
> _S_value
> >> =
> >> + constexpr static std::array<size_t, _Extents.size() + 1>
> _S_value =
> >> [] consteval
> >> {
> >> - constexpr size_t __rank = _Extents::rank();
> >> + constexpr size_t __rank = _Extents.size();
> >> std::array<size_t, __rank + 1> __ret;
> >> for(size_t __r = 0; __r < __rank + 1; ++__r)
> >> __ret[__r] = __static_prod<_Extents>(0, __r);
> >> @@ -213,13 +226,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> };
> >>
> >> // Pre-compute: \prod_{i = r+1}^n _Extents[i]
> >> - template<typename _Extents>
> >> + template<array _Extents>
> >> struct _RevProd
> >> {
> >> - constexpr static std::array<size_t, _Extents::rank()> _S_value =
> >> + constexpr static std::array<size_t, _Extents.size()> _S_value =
> >> [] consteval
> >> {
> >> - constexpr size_t __rank = _Extents::rank();
> >> + constexpr size_t __rank = _Extents.size();
> >> std::array<size_t, __rank> __ret;
> >> for(size_t __r = 0; __r < __rank; ++__r)
> >> __ret[__r] = __static_prod<_Extents>(__r + 1, __rank);
> >> @@ -228,10 +241,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> };
> >>
> >> template<typename _Extents>
> >> - constexpr span<const size_t>
> >> - __static_extents(size_t __begin = 0, size_t __end =
> >> _Extents::rank())
> >> + constexpr const array<size_t, _Extents::rank()>&
> >> + __static_extents()
> >> noexcept
> >> - { return _Extents::_S_storage::_S_static_extents(__begin,
> __end); }
> >> + { return _Extents::_S_storage::_S_static_extents(); }
> >>
> >> template<typename _Extents>
> >> constexpr span<const typename _Extents::index_type>
> >> @@ -358,8 +371,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> }
> >>
> >> private:
> >> - friend span<const size_t>
> >> - __mdspan::__static_extents<extents>(size_t, size_t);
> >> + friend const array<size_t, rank()>&
> >> + __mdspan::__static_extents<extents>();
> >>
> >> friend span<const index_type>
> >> __mdspan::__dynamic_extents<extents>(const extents&, size_t,
> >> size_t);
> >> @@ -384,6 +397,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> return false;
> >> }
> >>
> >> + template<typename _Tp, size_t _Nm>
> >>
> > Add [[__gnu__::__always_inline__]], as we do not want this to be emitted
> > even in debug mode.
> > We may want to do the same for __empty, __size, __fwd_prod and
> __rev__prod,
> > that uses
> > _Extents as template parameter.
> >
> >> + consteval bool
> >> + __contains_zero(const array<_Tp, _Nm>& __exts) noexcept
> >> + { return __contains_zero(span<const _Tp, _Nm>{__exts}); }
> >> +
> >> template<typename _Extents>
> >> constexpr bool
> >> __empty(const _Extents& __exts) noexcept
> >> @@ -412,7 +430,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> constexpr typename _Extents::index_type
> >> __fwd_prod(const _Extents& __exts, size_t __r) noexcept
> >> {
> >> - size_t __sta_prod = _FwdProd<_Extents>::_S_value[__r];
> >> + constexpr auto __sta_exts = __static_extents<_Extents>();
> >> + size_t __sta_prod = _FwdProd<__sta_exts>::_S_value[__r];
> >> return __extents_prod(__exts, __sta_prod, 0, __r);
> >> }
> >>
> >> @@ -420,8 +439,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> constexpr typename _Extents::index_type
> >> __rev_prod(const _Extents& __exts, size_t __r) noexcept
> >> {
> >> + constexpr auto __sta_exts = __static_extents<_Extents>();
> >> constexpr size_t __rank = _Extents::rank();
> >> - size_t __sta_prod = _RevProd<_Extents>::_S_value[__r];
> >> + size_t __sta_prod = _RevProd<__sta_exts>::_S_value[__r];
> >> return __extents_prod(__exts, __sta_prod, __r + 1, __rank);
> >> }
> >>
> >> --
> >> 2.50.0
> >>
> >>
> >
>
>
