[PATCH v2 1/5] libstdc++: Check prerequisites of layout_*::operator().
Luc Grosheintz
luc.grosheintz@gmail.com
Thu Jul 3 10:08:23 GMT 2025
On 7/1/25 22:56, Jonathan Wakely wrote:
> On Tue, 1 Jul 2025 at 11:32, Tomasz Kaminski <tkaminsk@redhat.com> wrote:
>>
>> Hi,
>> More of the review will be later, but I have noticed that you have added preconditions checks
>> to the layouts, and then avoid checking them inside the operator[] of the mdspan. This is general
>> sounds good.
>>
>> However, the precondition on mdspan::operator[] is now hardened: https://eel.is/c++draft/views.multidim#mdspan.mdspan.members-3.
>> This implies that breaking this precondition is a contract violation, and thus the user may expect a contract violation handler to be invoked
>> for it. Amongst the information provided to the handler via contract_violation object (https://eel.is/c++draft/support.contract.violation)
>> is source_location, that includes the name.
>
> Even without contracts, our __glibcxx_assert macro includes the
> __PRETTY_FUNCTION__ string in the abort message.
>
>> Given that I think we want to always check the extents in operator[] of mdspan, and thus remove
>> the checks from layout (to avoid duplication).
>
> I think it's probably QoI whether the contract checks happen directly
> in the operator[] function or in something that it calls. If doing the
> checks in operator[] has no extra runtime cost (e.g. due to them
> getting duplicated), then I think it is better to do them there, so
> that errors are reported from the "correct" place. But if it hurts
> performance, I don't think it's essential to check them there.
The reasoning for this approach was:
1. The mapping::operator() and mdspan::operator[] have the same
precondition; and mdspan::operator[] calls mapping::operator().
2. The place I chose to check the precondition is where we already
have both the index and the extent in L1 and almost certainly in a
register. The hope was that together with branch prediction, this
will be a reasonably cheap place to put the check.
3. The layouts are highly valuable on their own. I've implemented
that piece of logic numerous times in different contexts; and it's
wonderful that soon we can convert `i, j, k` to a linear index easily
using the standard library.
Therefore, I didn't want to skip them in mapping::operator() because
they're a guard against out of bounds accesses, e.g. in a user-defined
dynamically allocated, owning, multi-dimensional array.
4. Finally, my assumption was that for performance critical code
one would be forced to turn off bounds checks. Hence, any place that
doesn't duplicate the check would be acceptable (until measured
otherwise).
In HPC workloads it wouldn't be uncommon to have 1M elements and 1M
iterations and 10s - 100s of different accesses per element per
iteration; per CPU core. In this loop often there's little more than
a few additions and multiplications.
There's a few paths forwards:
1. Remove the check from mapping::operator() and unconditionally
check in mdspan::operator[].
2. Leave it as is and return when we do optimization or hardening.
3. Start measuring to figure out the cost of these checks; and then
decide.
I'm open to all three.
>
>
>>
>> Hope, the above makes sense.
>>
>> Regards,
>> Tomasz
>>
>> On Fri, Jun 27, 2025 at 11:12 AM Luc Grosheintz <luc.grosheintz@gmail.com> wrote:
>>>
>>> Previously the prerequisite that the arguments passed to operator() are
>>> a multi-dimensional index (of extents()) was not checked.
>>>
>>> This commit adds the __glibcxx_asserts and the required tests.
>>>
>>> libstdc++-v3/ChangeLog:
>>>
>>> * include/std/mdspan: Check prerequisites of
>>> layout_*::operator().
>>> * testsuite/23_containers/mdspan/layouts/class_mandate_neg.cc:
>>> Add tests for prerequisites.
>>>
>>> Signed-off-by: Luc Grosheintz <luc.grosheintz@gmail.com>
>>> ---
>>> libstdc++-v3/include/std/mdspan | 4 +++
>>> .../mdspan/layouts/class_mandate_neg.cc | 26 +++++++++++++++++++
>>> 2 files changed, 30 insertions(+)
>>>
>>> diff --git a/libstdc++-v3/include/std/mdspan b/libstdc++-v3/include/std/mdspan
>>> index c72a64094b7..39d02ac08df 100644
>>> --- a/libstdc++-v3/include/std/mdspan
>>> +++ b/libstdc++-v3/include/std/mdspan
>>> @@ -441,6 +441,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>> _IndexType __mult = 1;
>>> auto __update = [&, __pos = 0u](_IndexType __idx) mutable
>>> {
>>> + __glibcxx_assert(cmp_less(__idx, __exts.extent(__pos)));
>>> __res += __idx * __mult;
>>> __mult *= __exts.extent(__pos);
>>> ++__pos;
>>> @@ -651,6 +652,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>> auto __update = [&, __pos = __exts.rank()](_IndexType) mutable
>>> {
>>> --__pos;
>>> + __glibcxx_assert(cmp_less(__ind_arr[__pos],
>>> + __exts.extent(__pos)));
>>> __res += __ind_arr[__pos] * __mult;
>>> __mult *= __exts.extent(__pos);
>>> };
>>> @@ -822,6 +825,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>> {
>>> auto __update = [&, __pos = 0u](_IndexType __idx) mutable
>>> {
>>> + __glibcxx_assert(cmp_less(__idx, __m.extents().extent(__pos)));
>>> __res += __idx * __m.stride(__pos++);
>>> };
>>> (__update(__indices), ...);
>>> diff --git a/libstdc++-v3/testsuite/23_containers/mdspan/layouts/class_mandate_neg.cc b/libstdc++-v3/testsuite/23_containers/mdspan/layouts/class_mandate_neg.cc
>>> index 7091153daba..f5aab22aff0 100644
>>> --- a/libstdc++-v3/testsuite/23_containers/mdspan/layouts/class_mandate_neg.cc
>>> +++ b/libstdc++-v3/testsuite/23_containers/mdspan/layouts/class_mandate_neg.cc
>>> @@ -45,4 +45,30 @@ auto b5 = B<5, std::layout_stride, std::layout_right>(); // { dg-error "require
>>> auto b6 = B<6, std::layout_stride, std::layout_left>(); // { dg-error "required from" }
>>> auto b7 = B<7, std::layout_stride, std::layout_stride>(); // { dg-error "required from" }
>>>
>>> +template<typename Layout>
>>> + constexpr bool
>>> + test_linear_index_0d()
>>> + {
>>> + auto m = typename Layout::mapping<std::extents<int, 0>>{};
>>> + (void) m(0); // { dg-error "expansion of" }
>>> + return true;
>>> + }
>>> +static_assert(test_linear_index_0d<std::layout_left>()); // { dg-error "expansion of" }
>>> +static_assert(test_linear_index_0d<std::layout_right>()); // { dg-error "expansion of" }
>>> +static_assert(test_linear_index_0d<std::layout_stride>()); // { dg-error "expansion of" }
>>> +
>>> +template<typename Layout>
>>> + constexpr bool
>>> + test_linear_index_3d()
>>> + {
>>> + auto m = typename Layout::mapping<std::extents<int, 3, 5, 7>>{};
>>> + (void) m(2, 5, 5); // { dg-error "expansion of" }
>>> + return true;
>>> + }
>>> +static_assert(test_linear_index_3d<std::layout_left>()); // { dg-error "expansion of" }
>>> +static_assert(test_linear_index_3d<std::layout_right>()); // { dg-error "expansion of" }
>>> +static_assert(test_linear_index_3d<std::layout_stride>()); // { dg-error "expansion of" }
>>> +
>>> // { dg-prune-output "must be representable as index_type" }
>>> +// { dg-prune-output "non-constant condition for static assertion" }
>>> +// { dg-prune-output "__glibcxx_assert" }
>>> --
>>> 2.49.0
>>>
>
More information about the Libstdc++
mailing list