[PATCH v5 05/10] libstdc++: Implement layout_left from mdspan.
Luc Grosheintz
luc.grosheintz@gmail.com
Tue May 13 10:02:09 GMT 2025
On 5/12/25 6:02 PM, Tomasz Kaminski wrote:
> Thank you for all the work that you have done by doing the two
> implementations and
> extensive test cases. I wanted to respond to a few points that I think we
> may want
> to consider to be bugs in specification, and report them as bugs in
> standard.
> (Would you be interested in doing so?)
>
> I do not understand why the following is the case:
> - `layout_left::mapping<Extents> == layout_right::mapping<OExtents>`
> is valid, if and only iff `Extents != OExtents`.
> For the approach with bases, I was considering something less excessive
> where we only
> extract Rank0 and Rank1 bases, and not separation at each possible levels.
Yes, I'm interested in reporting them and I would like to continue the
discussion in a new thread:
https://gcc.gnu.org/pipermail/libstdc++/2025-May/061358.html
>
> Regarding, which commits to publish, given that:
> * your test showed no difference in the optimized binary
> * various inconsistencies in the current specification (we may want to
> address them)
> * your preference towards separate flat implementation
> I think you should move forward with each layout being totally separate.
>
I'll prepare and submit a new patch series with the flat
implementation. Please let me know if there's more that be checked.
> Regards,
> Tomasz
>
> On Mon, May 12, 2025 at 5:17 PM Luc Grosheintz <luc.grosheintz@gmail.com>
> wrote:
>
>>
>>
>> On 5/9/25 8:16 AM, Tomasz Kaminski wrote:
>>> The test I would perform would be :
>>> std::layout_left::mapping<std::extents<int>> l0;
>>> std::layout_right:mapping<std::extents<int>> r0;
>>> // stride
>>> bool all_unique()
>>> {
>>> return l0.is_unique();
>>> return r0.is_unique();
>>> }
>>> And we should have only one is_unique symbol.
>>>
>>> but with a lot more duplication. Then compile it with:
>>>> gcc -O2 -c many_symbols.cc
>>
>> I finished both implementation and wrote the type of test file we
>> agreed on, it calls all or almost all ctors, operator(), is_exhaustive,
>> extents() and stride. I compiled the test file against both
>> implementations (which both pass all the tests).
>>
>> The generated code in `.text` on `-g -O2` is exceedingly similar (often
>> identical) whether we use the flat implementation or the one using base
>> classes. Using `nm` I find no symbols (other than those for the global
>> variables and the functions to exercise the layout code), everything has
>> been inlined.
>>
>> When looking at code compiled with `-O0` I see more symbols with the
>> implementation that uses base classes to reduce the number of symbols. I
>> looked at the symbols and can confirm that the method `extents` exists
>> exactly once for each specialization of `std::extents` (and not once per
>> specialization per layout); same for `operator()`.
>>
>> However, when looking at ctors I find that (certain) inherited ctors are
>> given a symbol. As an example let's look at:
>>
>> ```
>> #include <mdspan>
>>
>> constexpr size_t dyn = std::dynamic_extent;
>>
>> using M1 = std::layout_left::mapping<std::extents<int, 5>>;
>> using M2 = std::layout_left::mapping<std::extents<char, 5>>;
>>
>> M1 ctor_cycle_1d(M2 m2) { return M1(m2); }
>> ```
>>
>> This results in the following three (demangled) symbols:
>>
>> std::layout_left::mapping<std::extents<int, 5ul> >
>> ::_Rank1_mapping_base<std::extents<char, 5ul> >(
>> std::__mdspan::_Rank1_mapping_base<std::extents<char, 5ul> > const&)
>>
>> std::__mdspan::_Mapping_base<std::extents<int, 5ul> >
>> ::_Mapping_base(std::extents<int, 5ul> const&)
>>
>> std::__mdspan::_Rank1_mapping_base<std::extents<int, 5ul> >
>> ::_Rank1_mapping_base<std::extents<char, 5ul> >(
>> std::__mdspan::_Rank1_mapping_base<std::extents<char, 5ul> > const&)
>>
>> (Naturally, there's more, e.g. for _ExtentsStorage and extents, etc.
>> Using `objdump -C -d` I can confirm that these "genuine" and there's a
>> little bit of code associated with each symbol.)
>>
>> With the current implementations I don't see much difference in object
>> file size, and almost one when using -O2.
>>
>> Total object file sizes:
>> naive | bases
>> -O2 9832 10240
>> -g -O2 250768 261920
>> -g -O0 600512 564400
>>
>> Number of symbols (nm -C OBJ | wc -l):
>>
>> naive | bases
>> -O2 46 46
>> -g -O2 46 46
>> -g -O0 816 907
>>
>> I hope this explains what I'm seeing. Please let me know if anything is
>> unclear or if you suspect I'm doing something wrong.
>>
>> --------------------------------------------------------------------
>>
>> I would like to once more make the case for the flat implementation,
>> i.e. one class per layout kind, by summarizing some of the twists I've
>> encountered.
>>
>> Generic oddities:
>> - `layout_left::mapping<Extents> == layout_right::mapping<OExtents>`
>> is valid, if and only iff `Extents != OExtents`.
>>
> - `layout_right(layout_stride)` is noexcept but
>> `layout_left(layout_stride)` isn't.
>>
> This inconsistency seems to be a bug in specification. I doubt this is
> intended.
>
>>
>> Traps:
>> - The ctor `mapping(extents_type)` can't be inherited, because if we
>> do, the template parameter `_Extents` isn't inferred.
>> - We must work around a compiler issue that prevents inheriting the
>> condition for explicitness of ctors.
>> - Reusing `_Rank0_mapping_base` for layout_stride requires hiding the
>> ctor `mapping(extents_type)`.
>>
> - For rank == 0, layout_stride is convertible from other
>> layout_stride, if and only if the extent_type's are convertible.
>> Whereas layout_stride is convertible to layout_{left,right}
>> unconditionally (at rank 0).
>>
> Here, I also believe that either we should consistently ignore or take into
> consideration
> the convertibility of index_types for the rank(). Currently it seems like
> we are in between
> these two.
>
>>
>> I believe I've forgotten one; and I'm definitely very worried I've not
>> uncovered all of them yet.
>>
>> Inconveniences:
>> - The ctor `mapping(layout_stride)` can't be shared between
>> layout_left and layout_right, if we want to respect the missing
>> `noexcept` for layout_left.
>>
> This seems like a bug in a standard.
>
>> - While we can extract a `_MappingBase` which deals with the `extents`
>> object we can't write the correct compatiblity check for
>> `layout_stride` and the two padded layouts at that level. (Meaning
>> we'll need an intermediate layer of ctors for checking these
>> conditions).
>> - The kind of the layout (left, right, stride) and the rank of the
>> layout appear as if they're orthogonal concepts. However, the two
>> concepts are intertwined. Making it hard to implement the notion of
>> Rank0 or Rank1 in a manner that's independent of the layout kind.
>> - The trait `std::is_convertible_v` doesn't actually check that
>> the code used to perform the conversion is valid; meaning one must
>> also make sure to instantiate the ctor (in all possible variations).
>> - I'm currently at over 1500 lines of code for testing "everything" in
>> all it's variations.
>>
>> The architecture at scale looks like this:
>>
>> - _Mapping_base: because we would like to extract the method
>> `extents`.
>> - _Rank0_mapping_base: for code shared between all three layouts: e.g.
>> ctors and required_span_size.
>> - _Rank0_left_right_base: for code not shared with stride: ctor from
>> layout_stride (because of differing explicitness conditions).
>> - _Rank1_mapping_base: for code shared between left and right: e.g.
>> layout changing ctors, trivial implementation of operator().
>> - _RankN_mapping_base: for code shared between left and right: e.g.
>> ctors that check their preconditions, required_span_size.
>> - _RankN_left_base: code for rank N and left only, e.g. non-layout
>> changing ctors, operator() and stride.
>> - _RankN_right_base: see _RankN_left_base.
>> - _Rank0_stride_base: because we need `stride()`.
>> - _Stride: which is a base which contains `_M_stride`. This we want
>> because it only depends on IndexType and `sizeof...(Indices)`.
>> - _RankN_stride_base: code for rank N and stride only, e.g. generic
>> implementation of operator() and required_span_size().
>> - _IsAlwaysStrided: for `is_always_strided` and `is_strided`, because
>> they don't depend on the extents.
>> - _IsAlwaysUnique: see above.
>> - _IsExhaustive<Always>: for `is_always_exhaustive` and (if Always)
>> `is_exhaustive`.
>>
>> Plus the three not entirely empty classes for `layout_left`,
>> `layout_right` and `layout_stride`.
>>
>> I'm happy to submit either version for review; they're both ready and
>> just need to be structured into individual commits.
>>
>>
>
More information about the Libstdc++
mailing list