[libcxx-commits] [PATCH] D154367: [libc++] mdspan - implement mdspan class

Christian Trott via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Wed Jul 19 19:37:09 PDT 2023


crtrott marked 23 inline comments as done.
crtrott added a comment.

Addressed all the comments, uploading patch now.



================
Comment at: libcxx/include/__mdspan/mdspan.h:65
+           class _Extents,
+           class _LayoutPolicy   = layout_right,
+           class _AccessorPolicy = default_accessor<_ElementType> >
----------------
ldionne wrote:
> Do you test that the default layout policy is `layout_right` and the default accessor policy is `default_accessor<_ElementType>`? You can do that with
> 
> ```
> static_assert(std::is_same_v<mdspan<T, E>, mdspan<T, E, layout_right, default_accessor<T>>>);
> ```
> 
> Should probably be in `types.pass.cpp` if not already there.
Added.


================
Comment at: libcxx/include/__mdspan/mdspan.h:136
+                is_default_constructible_v<mapping_type> && is_default_constructible_v<accessor_type>)
+  = default;
+  _LIBCPP_HIDE_FROM_ABI constexpr mdspan(const mdspan&) = default;
----------------
ldionne wrote:
> This raises an interesting question. The spec has preconditions (https://eel.is/c++draft/views.multidim#mdspan.mdspan.cons-2), but we can't check them if we make this `= default`. The `= default` is not required by the standard but it's arguably a nice property to have. This allows making `mdspan` trivially default constructible if the `data_handle_type`, `mapping_type` and `accessor_type` are trivially default constructible, right?
> 
> Actually, considering this a bit more, per https://eel.is/c++draft/views.multidim#mdspan.mdspan.cons-3 the effects are supposed to be: `Value-initializes ptr_, map_, and acc_.`. This is not the case if we make this `= default` and we have a `data_handle_type`, `mapping_type` or `accessor_type` that's an aggregate (not all of those might be possible but you get the point). So I think this is non-conforming. For example, if you have an aggregate `accessor_type` with a dummy `int` member, that `int` will never be value-initialized (zeroed-out) with your current implementation, but it should be according to the Standard. This needs a test.
Doesn't the {} make it so its value initialized?

```
  _LIBCPP_NO_UNIQUE_ADDRESS data_handle_type __ptr_{};
  _LIBCPP_NO_UNIQUE_ADDRESS mapping_type __map_{};
  _LIBCPP_NO_UNIQUE_ADDRESS accessor_type __acc_{};
```

I believe this makes it so that the three members are value initialized for the defaulted default constructor doesn't it?

Which means the only thing we are not doing is the uncheckable precondition.


================
Comment at: libcxx/include/__mdspan/mdspan.h:137-138
+  = default;
+  _LIBCPP_HIDE_FROM_ABI constexpr mdspan(const mdspan&) = default;
+  _LIBCPP_HIDE_FROM_ABI constexpr mdspan(mdspan&&)      = default;
+
----------------
ldionne wrote:
> Do you have tests that those are trivial operations if the members are trivially copy/move constructible?
Adding it.


================
Comment at: libcxx/include/__mdspan/mdspan.h:158
+  template < class _OtherIndexType, size_t _Size>
+    requires(is_convertible_v<_OtherIndexType, index_type> && is_nothrow_constructible_v<index_type, _OtherIndexType> &&
+             ((_Size == rank()) || (_Size == rank_dynamic())) && is_constructible_v<mapping_type, extents_type> &&
----------------
ldionne wrote:
> These requirements are wrong AFAICT. They should be using `const&` like the ones for `array` as described in https://eel.is/c++draft/views.multidim#mdspan.mdspan.cons-8.1.
> 
> I think what you want is `is_convertible_v<const _OtherIndexType&, index_type> && is_nothrow_constructible_v<index_type, const _OtherIndexType&>`.
> 
> Those need tests. If you have an index type where `index_type(_OtherIndexType&&)` works but `index_type(_OtherIndexType const&)` doesn't, then the `requires` before my comment will be satisfied but you will get a hard compiler error while trying to compile `__map_(extents_type(__exts))` below (I think). Please also add the same test for the `std::array` constructor above: if I go and remove `const&` from the equivalent `requires` above, a test should fail.
`index_type` is integral so I don't think we can write a test for that using the move thing? I.e. the only thing you a move into `int` is an `int`. However, we could test  that we have an OtherIndexType where the conversion operator to integral is non-const and/or missing no-except. Turns out that this was already wrong in `extents` itself, which I need to fix in order to test mdspan, because otherwise it can go through the constructor taking extents ...


================
Comment at: libcxx/include/__mdspan/mdspan.h:163
+      _LIBCPP_HIDE_FROM_ABI constexpr mdspan(data_handle_type __p, span<_OtherIndexType, _Size> __exts)
+      : __ptr_(std::move(__p)), __map_(extents_type(__exts)), __acc_{} {}
+
----------------
ldionne wrote:
> If you forget to `std::move` here (or in any of the similar constructors), which test fails? If there's none, please make sure to add coverage.
Added coverage via `move_counted_handle` i.e. a handle type which increments a static variable during move.


================
Comment at: libcxx/include/__mdspan/mdspan.h:166
+  _LIBCPP_HIDE_FROM_ABI constexpr mdspan(data_handle_type __p, const extents_type& __exts)
+    requires(is_default_constructible_v<accessor_type> && is_constructible_v<mapping_type, extents_type>)
+      : __ptr_(std::move(__p)), __map_(__exts), __acc_{} {}
----------------
ldionne wrote:
> This needs a test, which should be possible if you have an `extents_type` where `mapping_type(extents_type&&)` works but `mapping_type(extents_type const&)` doesn't. Then the `requires` before my comment would enable this `mdspan` constructor, but you'd get a hard compiler error while trying to compile `__map_(__exts)` below.
Fixed and added test by modifying the `layout_wrapped_integral` depending on value of the _Wrap it now has a  constructor from `extents_type.&&` Note that construction from extents and construction from array/span/integers has different requirements for the mapping. We are now testing both.


================
Comment at: libcxx/include/__mdspan/mdspan.h:177-178
+  template < class _OtherElementType, class _OtherExtents, class _OtherLayoutPolicy, class _OtherAccessor>
+    requires(is_constructible_v<mapping_type, typename _OtherLayoutPolicy::template mapping<_OtherExtents>> &&
+             is_constructible_v<accessor_type, _OtherAccessor>)
+  explicit(!is_convertible_v<const typename _OtherLayoutPolicy::template mapping<_OtherExtents>&, mapping_type> ||
----------------
ldionne wrote:
> Needs tests.
Added a whole bunch of testing, and made sure (i.e. tried) that any combination of missing const& both for the requires and the explicit will result in failure. 


================
Comment at: libcxx/include/__mdspan/mdspan.h:223
+  _LIBCPP_HIDE_FROM_ABI constexpr reference operator[](const array< _OtherIndexType, rank()>& __indices) const {
+    return __idx_fold<>::template __callop<reference>(*this, __indices);
+  }
----------------
ldionne wrote:
> Is there any reason why we're doing this here instead of following the standard wording more closely? Is it because you want to avoid calling the variadic version in case you have a large number of dimensions?
> 
> I would inline all the uses of `__idx_fold` into their calling functions and use a lambda instead, that's easier to read. Otherwise the implementation of the function is basically 100+ lines above the function itself.
Yeah, we can't avoid having an expansion happening in this operator via idx_fold (or now changed via a lambda) but we don't have to instantiate another variadic function here. We have ongoing issues with inlining depth in our big applications, and so we make judgement calls regarding delegation.

This is the lambda version body of the function (a bit more than the fold expression but ok:
```
    return __acc_.access(__ptr_, [&]<size_t ... _Idxs>(index_sequence<_Idxs...>) {
      return __map_(__indices[_Idxs]...);
    }(make_index_sequence<rank()>()));
```

This is the forwarding version:
```
    return [&, self = this]<size_t ... _Idxs>(index_sequence<_Idxs...>) -> reference {
       return (*self)[__indices[_Idxs]...];
    }(make_index_sequence<rank()>());
```

I personally don't see that as better, we need to explicitly capture this, and need to define the return type (otherwise it returns a value not a reference. 


================
Comment at: libcxx/include/__mdspan/mdspan.h:229
+  _LIBCPP_HIDE_FROM_ABI constexpr reference operator[](span<_OtherIndexType, rank()> __indices) const {
+    return __idx_fold<>::template __callop<reference>(*this, __indices);
+  }
----------------
ldionne wrote:
> Note to self: I have not actually reviewed the implementation of these `idx_fold` operations yet.
Removed it.


================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/ctor.dh_array.pass.cpp:48-51
+template <class MDS, class Exts>
+concept check_mdspan_ctor_implicit = requires(MDS m, typename MDS::data_handle_type h, const Exts& exts) {
+  m = {h, exts};
+};
----------------
ldionne wrote:
> I don't think that works. The usual pattern we use here is
> 
> ```
> template <class T>
> void accept(T);
> 
> template <class MDS, class Exts>
> concept check_mdspan_ctor_implicit = requires(typename MDS::data_handle_type h, const Exts& exts) {
>   accept<MDS>({h, exts});
> };
> ```
> 
> You'll want to make all of your explicit tests consistent.
I don't think that works, because it's multi argument explicitlness. I.e. this doesn't even compile:

```
class Bar {
  constexpr Bar(int, int) {}
};

template<class T>
constexpr bool accept(T) { return true; }

static_assert(accept({1,1}));
```

My understanding is that for multi argument constructors the ONLY difference in whether you have an explicit or not is whether

```
T a = {....};
```
works.

It doesn't make any difference for function calls.


================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/properties.pass.cpp:98
+  ASSERT_NOEXCEPT(m.rank());
+  static_assert(MDS::rank() == MDS::extents_type::rank());
+
----------------
ldionne wrote:
> Those static functions should be `assert(...)`ed instead. Then we call those from a constexpr and a non-constexpr context.
> 
> Right now the following implementation passes your tests:
> 
> ```
> static constexpr rank_type rank() noexcept {
>   if (std::is_constant_evaluated()) return extents_type::rank();
>   else return 0;
> }
> ```
> 
> I think all the "properties" are affected.
Yeah done. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154367/new/

https://reviews.llvm.org/D154367



More information about the libcxx-commits mailing list