On Thu, Jul 3, 2025 at 12:08 PM Luc Grosheintz <luc.groshei...@gmail.com> wrote:
> > > On 7/1/25 22:56, Jonathan Wakely wrote: > > On Tue, 1 Jul 2025 at 11:32, Tomasz Kaminski <tkami...@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. > I think such types would have their own bounds checks, contracts, preconditions. > > 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[]. > I would go for option 1. > > 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.groshei...@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.groshei...@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 > >>> > > > >
