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.
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.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
