On Thu, 3 Jul 2025 at 11:12, Tomasz Kaminski <tkami...@redhat.com> wrote: > > > > 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().
Yes, although a user-defined mapping might not bother to check preconditions. So in order for us to implement the required check in operator[] we really need to check it there. We could check it in *both* places, and assume that the compiler will see that the second check is entirely redundant. We could also check in mapping::operator() and then in mdspan::operator[] do something like: if constexpr (!__is_std_mapping<mapping_type>) __glibcxx_assert(...); So if we know there's a check in mapping::operator() then don't bother to check in operator[] as well. >> 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. Not if it's just something wrapping a unique_ptr<T[]>, for example. >> >> >> 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. My original preference was option 2, but I've convinced myself that we need the checks in operator[]. Rather than repeat them in both places, I think I'm OK with option 1 too. >> 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.
