On Fri, May 23, 2025 at 4:22 PM Luc Grosheintz <luc.groshei...@gmail.com>
wrote:
>
>
> On 5/22/25 15:21, Tomasz Kaminski wrote:
> > Thanks for working on the patches, they look solid, comments.
> >
> > Could you prepare a separate patch to fix initialization
> > default-initialization of extents,
> > that you have noticed, standard requires them to be value-initialized,
> and
> > add corresponding test?
>
> Yes, I was thinking of something that statically allocates
> sizeof(Extents) bytes, sets all the bits to 1 and then uses placement
> new to "run" the default ctor. That way we can be confident that
> the zeros we're observing aren't accidental.
>
Running the constructor at compile time will be sufficient, to detect if
zero were
set explicitly, so do not need to go extra.
>
> We do a renaming within this patch series that affects the value
> initialization patch. Should I submit a v4 that applies cleanly
> to master *after* applying the value initialization patch? (And
> cause conflict if applied to the current state of master.)
>
Yes, I think we will land the initialization patch before the rest of
series..
>
> >
> > Similarly, we have test for default constructor of stride_mapping, I
> would
> > add them for other layouts,
> > and check:
> > * all extents being static -> depends on mapping
> > * first extent being dynamic, and rest static -> all strides are zero
> > * middle extent being dynamic
>
> Yes, definitely.
>
> >
> > For the stride and product computation, we should perform them in
> > Extent::size_type, not index_type.
> > The latter may be signed, and we may hit UB in multiplying non-zero
> > extents, before reaching the zero.
>
> This seems unfortunate, because if I understood correctly, the idea
> behind allowing signed `index_type`, was that signed integer operations
> are faster (exactly because they don't need to behave "well" on overflow).
>
> I've made the changes to use unsigned integers. However, I'd like to add
> this to my pile of performance questions to be investigated.
>
Just to clarify, we only need to perform stride/required_span_size
computation
using the size_type. For the operator() we should keep using an index_type,
as:
* we check that required_span_size() fits in index_type
* if any extent is zero the operator() cannot be invoked.
This will we will still use signed (potentially faster) arithmetic when
retrieving elements,
and unsigned will be limited to use of stride, and other funciton that
perform layout conversions.
And I am less concerned with them.
>
> I believe that since constexpr code must be UB free, we should be able
> to create tests, that can detect overflow in the naive formulas.
>
> >
> > For is_exhaustive question, I will write to LWG reflector to ask authors,
> > and see what their opinion is.
> > Will keep you posted.
>
> Thank you! Somehow I'm failing to figure out a reasonable algorithm
> to check it in a conformant way, so this is definitely welcome.
> (Brute-force is not reasonable.)
>
I should soon have LWG issue number. Then you will add comment in
implementation:
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// XXX. Issue title
And also mention in commit message that you are implemented it as modified
by the issue.
>
> >
> > In a lot of tests we are doing, where I believe we could skip template
> > parameters, and deduce it for argument.
> > verify_nothrow_convertible<Layout, std::extents<int>>(
> > + std::extents<int>{});
> > Could you look into doing it?
>
> I could only find them in cases like:
>
> template<typename Layout, typename Extents, typename OExtents>
> const void
> verify_nothrow_convertible(OExtents oexts);
>
> i.e. the first is explicitly given and the second is deduced. This
> happens when checking properties of mappings of the same layout with
> different extent_types. I didn't add anything to reduce the amount
> of code when Extents == OExtents, because it felt non-uniform (I
> don't like remembering special cases) and a little error prone. Let
> me know if you see it differently and would like me to add a default
> template argument to handle the case Extents == OExtents.
>
In that case, I believe this is OK.
>
> >
> > Regards,
> > Tomasz
> >
> > On Thu, May 22, 2025 at 2:21 PM Tomasz Kaminski <tkami...@redhat.com>
> wrote:
> >
> >>
> >>
> >> On Wed, May 21, 2025 at 4:21 PM Luc Grosheintz <
> luc.groshei...@gmail.com>
> >> wrote:
> >>
> >>> It's missing the "registration" of the three new classes in
> >>> std.cc.in.
> >>>
> >> Please remember to add it in next revisions.
> >>
> >>>
> >>> On 5/21/25 11:40, Luc Grosheintz wrote:
> >>>> Follows up on:
> >>>> https://gcc.gnu.org/pipermail/libstdc++/2025-May/061535.html
> >>>>
> >>>> To improve naming conventions, this series includes three new commits:
> >>>> * Two commits to rename _ExtentsStorage::_M_dynamic_extents, and
> >>>> extents::_M_dynamic_extents.
> >>>> * One commit to cleanup whitespace errors in extents.
> >>>>
> >>>> The changes to the existing commits are:
> >>>> * Fix division by zero bug.
> >>>> * Rename subextents -> extents.
> >>>> * Default arguments for __{static,dynamic}_extents.
> >>>> * Default argument for __static_quotient.
> >>>> * Four times: use range-based for.
> >>>> * Eliminate __has_static_zero
> >>>> * Short-circuit in __static_quotient.
> >>>> * Optimize __exts_prod for rank == rank_dynamic.
> >>>>
> >>>> This review suggestion was intentionally skipped:
> >>>> * Inline helper of __exts_prod, because with the additional
> >>>> optimization for rank == rank_dynamic, having two separate
> >>>> functions makes the highlevel structure a little bit more
> >>>> obvious. Additionally, there's numerous changes planned that
> >>>> might make one of the two functions much more verbose.
> >>>>
> >>>> Luc Grosheintz (9):
> >>>> libstdc++: Rename _ExtentsStorage::_M_dynamic_extents.
> >>>> libstdc++: Rename extents::_M_dynamic_extents.
> >>>> libstdc++: Cleanup formatting in mdspan.
> >>>> libstdc++: Implement layout_left from mdspan.
> >>>> libstdc++: Add tests for layout_left.
> >>>> libstdc++: Implement layout_right from mdspan.
> >>>> libstdc++: Add tests for layout_right.
> >>>> libstdc++: Implement layout_stride from mdspan.
> >>>> libstdc++: Add tests for layout_stride.
> >>>>
> >>>> libstdc++-v3/include/std/mdspan | 692
> +++++++++++++++++-
> >>>> .../mdspan/layouts/class_mandate_neg.cc | 42 ++
> >>>> .../23_containers/mdspan/layouts/ctors.cc | 401 ++++++++++
> >>>> .../23_containers/mdspan/layouts/mapping.cc | 569 ++++++++++++++
> >>>> .../23_containers/mdspan/layouts/stride.cc | 494 +++++++++++++
> >>>> 5 files changed, 2185 insertions(+), 13 deletions(-)
> >>>> create mode 100644
> >>>
> libstdc++-v3/testsuite/23_containers/mdspan/layouts/class_mandate_neg.cc
> >>>> create mode 100644
> >>> libstdc++-v3/testsuite/23_containers/mdspan/layouts/ctors.cc
> >>>> create mode 100644
> >>> libstdc++-v3/testsuite/23_containers/mdspan/layouts/mapping.cc
> >>>> create mode 100644
> >>> libstdc++-v3/testsuite/23_containers/mdspan/layouts/stride.cc
> >>>>
> >>>
> >>>
> >
>
>
