On Fri, May 23, 2025 at 5:25 PM Tomasz Kaminski <tkami...@redhat.com> wrote:
> > > 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. > The issue number is https://cplusplus.github.io/LWG/issue4266. I suggested also adjusting is_always_exhaustive, so is_exhaustive can be implemented as: if constexpr (!is_always_exhaustive()) if (size_t __size = __mdspan::__fwd_prod(_M_extents, extents_type::rank()) return __size == required_span_size(); return true; > >> > >> > 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 >> >>>> >> >>> >> >>> >> > >> >>
