cplusplus / nbballot

Handling of NB comments in response to ballots
14 stars 4 forks source link

US 45-106 24.7.8.3 [mdspan.accessor.default] Configurable index_type for default_accessor #522

Closed wg21bot closed 1 year ago

wg21bot commented 1 year ago

default_accessor’s access and offset members take their index argument as a size_t. As discussed in P2553, on certain architectures, register space is precious and 64-bit integer computations may be significantly [s]lower than 32-bit integer computations. That is why we [made] the index type of mdspan, extents, and layout mappings controllable as per P2553. However, we neglected to make the index type controllable for default_accessor.

Proposed change:

Modify the synopsis for default_accessor in 24.7.8.3 [mdspan.accessor.default] as follows:

namespace std {

template<class ElementType,
         **class IndexType = size_t**>
struct default_accessor {
  using offset_policy = default_accessor;
  using element_type = ElementType;
  using reference = ElementType&;
  using data_handle_type = ElementType*;
  **using index_type = IndexType;**

  constexpr default_accessor() noexcept = 
    default;
  template<class OtherElementType>
  constexpr default_accessor(
    default_accessor<OtherElementType>)
    noexcept;
  constexpr reference access(
    data_handle_type p,
    size_t**index_type** i) const noexcept;
  constexpr data_handle_type offset(
    data_handle_type p,
    size_t**index_type** i) const noexcept;
  };
}
FabioFracassi commented 1 year ago

2022-11-09 08:00 to 09:45 UTC-10 Kona Library Evolution Meeting

2022-11-09 08:00 to 09:45 UTC-10 Kona Library Evolution Minutes

Champion: Bryce Lelbach

Chair: Fabio Fracassi

Summary

This needs analysis of the implications of the changed requirements

Next Steps

Produce a paper with the discussion

crtrott commented 1 year ago

I did some experiments on this, but didn't have time to write a paper yet. Basically my conclusion is that this change is not needed. Here is code and some perf data on a few archs: https://github.com/kokkos/code-examples/tree/main/stdcpp-experiments/mdspan/default_accessor_with_index_type

I don't know whether I will be able to produce a paper in time for Issaquah.

brycelelbach commented 1 year ago

2023-02-06 10:30 to 12:00 Issaquah Library Evolution Meeting

US-45-106-114: Configurable index_type for default_accessor

2023-02-06 10:30 to 12:00 UTC-8 Issaquah Library Evolution Minutes

Champion: Mark Hoemmen (R)

Chair: Bryce Adelstein Lelbach (IP) & Ben Craig (IP)

Minute Taker: Steve Downey (IP)

Start: 2023-02-06 10:52 UTC-8

A performance evaluation was performed and it was determined that changing the accessor index type did not have an impact on three different architectures.

POLL: Reject C++23 National Body comment US-45-106 (Configurable index_type for default_accessor).

Strongly Favor Weakly Favor Neutral Weakly Against Strongly Against
7 14 2 1 0

Attendance: 20 (in person) + 10 (remote)

# of Authors: 1

Author Position: WF

Outcome: Consensus in favor.

WA: The more genericity, the better.

End: 11:00

Next Steps

Reject C++23 National Body comment US-45-106 (Configurable index_type for default_accessor).

jensmaurer commented 1 year ago

Rejected. There was no consensus for a change.