[v4,0/5] libstdc++: submdspan (part 2)

Message ID 20251204212846.248887-1-luc.grosheintz@gmail.com
Headers
Series libstdc++: submdspan (part 2) |

Message

Luc Grosheintz Dec. 4, 2025, 9:28 p.m. UTC
  These are the revised  submdspan_mapping related patches from v2.

A few comments:

  - __any_past_the_end: the suggestion was to precheck for an empty
    extent. I kept the current logic, because the additional complexity
    is only at compile time (there was a `constexpr` missing on a `else
    if` branch in v2). At runtime, it's a very simple check:
      (__slice_begin[k] == exts.extent[k]) || ...
    Even if the pre-check would make the condition simpler, for regular
    (non-empty) cases we'd be forced to first check for empty, then
    check for out-of-bounds. Unless I'm missing something, that's
    strictly worse.

    The main motivation here is to eliminate these checks when the slice
    is a collapsing slice, because these checks adds a "lot" of code
    compared to the hand-rolled version. (With the current
    implementation: submdspan(m, i, j, k) and mdspan(&m[i, j, k], E{})
    generate the same code, with E = std::extents<int>).

  - As proposed during review, the code to figure out the layout mapping
    type given a combination of slices has been rewritten. Essentially,
    we have a `enum class _SliceKind` what's (forced to be) categorical.
    From there one can use for-loops and immediate (consteval) functions
    insteads of traditional recursive structs.

    The twist here is that a __full slice is not a __unit_strided_slice
    slice. This is because: I don't want to use weak/plain enums;
    strong enums (enum class) don't do bitwise operations without
    implementing a lot of boilerplate; bitset feels inappropriate.
    Hence, this solution felt the one with the least boilerplate.
    Again, I might be missing a trick.

  - In __substrides_standardized I chose not use _M_strides, because
    _M_strides has size `rank`; while __substrides_standardized only
    returns an array of lenght `__subrank`. The difference can be seen,
    e.g., for submdspan(m, 1, 2, 3, full) which (technically) performs 3
    multiplications when zero are needed. Therefore, one would rely on
    the optimizer to eliminate the extra operation. It might be
    recognizable as dead code, but I've not checked.

  - Let me know if you want me to move the __subextents code in a
    separate commit.

  - I started following the suggestion to always use qualified names for
    functions. Let me know if you want to qualify everything (typenames,
    concepts, etc.)

Luc Grosheintz (5):
  libstdc++: Implement submdspan and submdspan_mapping for layout_left.
    [PR110352]
  libstdc++: Implement submdspan_mapping for layout_right. [PR110352]
  libstdc++: Implement submdspan_mapping for layout_stride. [PR110352]
  libstdc++: Implement submdspan_mapping for layout_left_padded.
    [PR110352]
  libstdc++: Implement submdspan_mapping for layout_right_padded.
    [PR110352]

 libstdc++-v3/include/std/mdspan               | 713 ++++++++++++++++--
 libstdc++-v3/src/c++23/std.cc.in              |   2 +-
 .../23_containers/mdspan/layout_traits.h      |   4 +
 .../mdspan/submdspan/submdspan.cc             | 377 +++++++++
 .../mdspan/submdspan/submdspan_mapping.cc     | 301 ++++++++
 .../mdspan/submdspan/submdspan_neg.cc         | 179 +++++
 6 files changed, 1514 insertions(+), 62 deletions(-)
 create mode 100644 libstdc++-v3/testsuite/23_containers/mdspan/submdspan/submdspan.cc
 create mode 100644 libstdc++-v3/testsuite/23_containers/mdspan/submdspan/submdspan_mapping.cc
 create mode 100644 libstdc++-v3/testsuite/23_containers/mdspan/submdspan/submdspan_neg.cc
  

Comments

Tomasz Kamiński Dec. 5, 2025, 9:29 a.m. UTC | #1
On Thu, Dec 4, 2025 at 10:27 PM Luc Grosheintz <luc.grosheintz@gmail.com>
wrote:

> These are the revised  submdspan_mapping related patches from v2.
>
I will try to review it fully today.

>
> A few comments:
>
>   - __any_past_the_end: the suggestion was to precheck for an empty
>     extent. I kept the current logic, because the additional complexity
>     is only at compile time (there was a `constexpr` missing on a `else
>     if` branch in v2). At runtime, it's a very simple check:
>       (__slice_begin[k] == exts.extent[k]) || ...
>     Even if the pre-check would make the condition simpler, for regular
>     (non-empty) cases we'd be forced to first check for empty, then
>     check for out-of-bounds. Unless I'm missing something, that's
>     strictly worse.
>
Yes, after some time of break I think you are making the right call here.

>
>     The main motivation here is to eliminate these checks when the slice
>     is a collapsing slice, because these checks adds a "lot" of code
>     compared to the hand-rolled version. (With the current
>     implementation: submdspan(m, i, j, k) and mdspan(&m[i, j, k], E{})
>     generate the same code, with E = std::extents<int>).
>
>   - As proposed during review, the code to figure out the layout mapping
>     type given a combination of slices has been rewritten. Essentially,
>     we have a `enum class _SliceKind` what's (forced to be) categorical.
>     From there one can use for-loops and immediate (consteval) functions
>     insteads of traditional recursive structs.
>
It looks much more readable now, only leaving small comments about not using
_Size as template parameter.

>
>     The twist here is that a __full slice is not a __unit_strided_slice
>     slice. This is because: I don't want to use weak/plain enums;
>     strong enums (enum class) don't do bitwise operations without
>     implementing a lot of boilerplate; bitset feels inappropriate.
>     Hence, this solution felt the one with the least boilerplate.
>     Again, I might be missing a trick.
>
>   - In __substrides_standardized I chose not use _M_strides, because
>     _M_strides has size `rank`; while __substrides_standardized only
>     returns an array of lenght `__subrank`. The difference can be seen,
>     e.g., for submdspan(m, 1, 2, 3, full) which (technically) performs 3
>     multiplications when zero are needed. Therefore, one would rely on
>     the optimizer to eliminate the extra operation. It might be
>     recognizable as dead code, but I've not checked.
>
Yes, it makes sense.
>
>
>   - Let me know if you want me to move the __subextents code in a
>     separate commit.
>
Not necessary.

>
>   - I started following the suggestion to always use qualified names for
>     functions. Let me know if you want to qualify everything (typenames,
>     concepts, etc.)
>
We qualify functions to avoid the compiler doing ADL, even if it will
result in only a
single candidate. No need to qualify anything else.

>
> Luc Grosheintz (5):
>   libstdc++: Implement submdspan and submdspan_mapping for layout_left.
>     [PR110352]
>   libstdc++: Implement submdspan_mapping for layout_right. [PR110352]
>   libstdc++: Implement submdspan_mapping for layout_stride. [PR110352]
>   libstdc++: Implement submdspan_mapping for layout_left_padded.
>     [PR110352]
>   libstdc++: Implement submdspan_mapping for layout_right_padded.
>     [PR110352]
>
>  libstdc++-v3/include/std/mdspan               | 713 ++++++++++++++++--
>  libstdc++-v3/src/c++23/std.cc.in              |   2 +-
>  .../23_containers/mdspan/layout_traits.h      |   4 +
>  .../mdspan/submdspan/submdspan.cc             | 377 +++++++++
>  .../mdspan/submdspan/submdspan_mapping.cc     | 301 ++++++++
>  .../mdspan/submdspan/submdspan_neg.cc         | 179 +++++
>  6 files changed, 1514 insertions(+), 62 deletions(-)
>  create mode 100644
> libstdc++-v3/testsuite/23_containers/mdspan/submdspan/submdspan.cc
>  create mode 100644
> libstdc++-v3/testsuite/23_containers/mdspan/submdspan/submdspan_mapping.cc
>  create mode 100644
> libstdc++-v3/testsuite/23_containers/mdspan/submdspan/submdspan_neg.cc
>
> --
> 2.52.0
>
>
  
Tomasz Kamiński Dec. 5, 2025, 2:08 p.m. UTC | #2
The implementation looks solid now, only very small comments there.
Please add a patch updating feature test macro, my preference would be to
keep internal padded layouts FTM and give it a value 202403, with comment
that corresponds to new layouts part of
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p2642r6.pdf.

I assume this is because, you were unsure about the value of FTM,
fortunately sd-6
(
https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations
)
already list the value for paper 202411.

Regards,
Tomasz

On Fri, Dec 5, 2025 at 10:29 AM Tomasz Kaminski <tkaminsk@redhat.com> wrote:

>
>
> On Thu, Dec 4, 2025 at 10:27 PM Luc Grosheintz <luc.grosheintz@gmail.com>
> wrote:
>
>> These are the revised  submdspan_mapping related patches from v2.
>>
> I will try to review it fully today.
>
>>
>> A few comments:
>>
>>   - __any_past_the_end: the suggestion was to precheck for an empty
>>     extent. I kept the current logic, because the additional complexity
>>     is only at compile time (there was a `constexpr` missing on a `else
>>     if` branch in v2). At runtime, it's a very simple check:
>>       (__slice_begin[k] == exts.extent[k]) || ...
>>     Even if the pre-check would make the condition simpler, for regular
>>     (non-empty) cases we'd be forced to first check for empty, then
>>     check for out-of-bounds. Unless I'm missing something, that's
>>     strictly worse.
>>
> Yes, after some time of break I think you are making the right call here.
>
>>
>>     The main motivation here is to eliminate these checks when the slice
>>     is a collapsing slice, because these checks adds a "lot" of code
>>     compared to the hand-rolled version. (With the current
>>     implementation: submdspan(m, i, j, k) and mdspan(&m[i, j, k], E{})
>>     generate the same code, with E = std::extents<int>).
>>
>>   - As proposed during review, the code to figure out the layout mapping
>>     type given a combination of slices has been rewritten. Essentially,
>>     we have a `enum class _SliceKind` what's (forced to be) categorical.
>>     From there one can use for-loops and immediate (consteval) functions
>>     insteads of traditional recursive structs.
>>
> It looks much more readable now, only leaving small comments about not
> using
> _Size as template parameter.
>
>>
>>     The twist here is that a __full slice is not a __unit_strided_slice
>>     slice. This is because: I don't want to use weak/plain enums;
>>     strong enums (enum class) don't do bitwise operations without
>>     implementing a lot of boilerplate; bitset feels inappropriate.
>>     Hence, this solution felt the one with the least boilerplate.
>>     Again, I might be missing a trick.
>>
>>   - In __substrides_standardized I chose not use _M_strides, because
>>     _M_strides has size `rank`; while __substrides_standardized only
>>     returns an array of lenght `__subrank`. The difference can be seen,
>>     e.g., for submdspan(m, 1, 2, 3, full) which (technically) performs 3
>>     multiplications when zero are needed. Therefore, one would rely on
>>     the optimizer to eliminate the extra operation. It might be
>>     recognizable as dead code, but I've not checked.
>>
> Yes, it makes sense.
>>
>>
>>   - Let me know if you want me to move the __subextents code in a
>>     separate commit.
>>
> Not necessary.
>
>>
>>   - I started following the suggestion to always use qualified names for
>>     functions. Let me know if you want to qualify everything (typenames,
>>     concepts, etc.)
>>
> We qualify functions to avoid the compiler doing ADL, even if it will
> result in only a
> single candidate. No need to qualify anything else.
>
>>
>> Luc Grosheintz (5):
>>   libstdc++: Implement submdspan and submdspan_mapping for layout_left.
>>     [PR110352]
>>   libstdc++: Implement submdspan_mapping for layout_right. [PR110352]
>>   libstdc++: Implement submdspan_mapping for layout_stride. [PR110352]
>>   libstdc++: Implement submdspan_mapping for layout_left_padded.
>>     [PR110352]
>>   libstdc++: Implement submdspan_mapping for layout_right_padded.
>>     [PR110352]
>>
>>  libstdc++-v3/include/std/mdspan               | 713 ++++++++++++++++--
>>  libstdc++-v3/src/c++23/std.cc.in              |   2 +-
>>  .../23_containers/mdspan/layout_traits.h      |   4 +
>>  .../mdspan/submdspan/submdspan.cc             | 377 +++++++++
>>  .../mdspan/submdspan/submdspan_mapping.cc     | 301 ++++++++
>>  .../mdspan/submdspan/submdspan_neg.cc         | 179 +++++
>>  6 files changed, 1514 insertions(+), 62 deletions(-)
>>  create mode 100644
>> libstdc++-v3/testsuite/23_containers/mdspan/submdspan/submdspan.cc
>>  create mode 100644
>> libstdc++-v3/testsuite/23_containers/mdspan/submdspan/submdspan_mapping.cc
>>  create mode 100644
>> libstdc++-v3/testsuite/23_containers/mdspan/submdspan/submdspan_neg.cc
>>
>> --
>> 2.52.0
>>
>>