[libcxx-commits] [PATCH] D154367: [libc++] mdspan - implement mdspan class
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Jul 18 12:18:59 PDT 2023
ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.
================
Comment at: libcxx/include/__mdspan/mdspan.h:65
+ class _Extents,
+ class _LayoutPolicy = layout_right,
+ class _AccessorPolicy = default_accessor<_ElementType> >
----------------
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.
================
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;
----------------
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.
================
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;
+
----------------
Do you have tests that those are trivial operations if the members are trivially copy/move constructible?
================
Comment at: libcxx/include/__mdspan/mdspan.h:157
+
+ template < class _OtherIndexType, size_t _Size>
+ requires(is_convertible_v<_OtherIndexType, index_type> && is_nothrow_constructible_v<index_type, _OtherIndexType> &&
----------------
There's a few instances of that.
================
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> &&
----------------
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.
================
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_{} {}
+
----------------
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.
================
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_{} {}
----------------
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.
================
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> ||
----------------
Needs tests.
================
Comment at: libcxx/include/__mdspan/mdspan.h:184-185
+ : __ptr_(__other.__ptr_), __map_(__other.__map_), __acc_(__other.__acc_) {
+ static_assert(is_constructible_v<data_handle_type, typename _OtherAccessor::data_handle_type>,
+ "mdspan: incompatible data_handle_type for mdspan construction");
+ static_assert(
----------------
================
Comment at: libcxx/include/__mdspan/mdspan.h:190
+ // The following precondition is part of the standard, but is unlikely to be triggered.
+ // The extents constructor checkes this and the mapping must be storing the extents, since
+ // its extents() function returns a const reference to extents_type.
----------------
================
Comment at: libcxx/include/__mdspan/mdspan.h:192-193
+ // its extents() function returns a const reference to extents_type.
+ // The only way this can be triggered is if the mapping conversion constructor would for example
+ // construct its extents() always only from the dynamic extents, instead of from the other extents.
+ if constexpr (rank() > 0) {
----------------
================
Comment at: libcxx/include/__mdspan/mdspan.h:204-205
+
+ _LIBCPP_HIDE_FROM_ABI constexpr mdspan& operator=(const mdspan&) = default;
+ _LIBCPP_HIDE_FROM_ABI constexpr mdspan& operator=(mdspan&&) = default;
+
----------------
Same question about testing for triviality.
================
Comment at: libcxx/include/__mdspan/mdspan.h:208
+ //--------------------------------------------------------------------------------
+ // [mdspan.basic.mapping], mdspan mapping domain multidimensional index to access codomain element
+
----------------
These "stable" names seem to originate from a previous version of the standard. https://eel.is/c++draft/views.multidim#mdspan doesn't seem to have the same names.
================
Comment at: libcxx/include/__mdspan/mdspan.h:213
+ (is_nothrow_constructible_v<index_type, _OtherIndexTypes> && ...) &&
+ (rank() == sizeof...(_OtherIndexTypes)))
+ _LIBCPP_HIDE_FROM_ABI constexpr reference operator[](_OtherIndexTypes... __indices) const {
----------------
Just for consistency with the spec.
================
Comment at: libcxx/include/__mdspan/mdspan.h:215
+ _LIBCPP_HIDE_FROM_ABI constexpr reference operator[](_OtherIndexTypes... __indices) const {
+ _LIBCPP_ASSERT(__mdspan_detail::__is_multidimensional_index_in(extents(), __indices...),
+ "mdspan: operator[] out of bounds access");
----------------
We have an open patch to categorize those.
================
Comment at: libcxx/include/__mdspan/mdspan.h:221
+ template < class _OtherIndexType>
+ requires(is_convertible_v<_OtherIndexType, index_type> && is_nothrow_constructible_v<index_type, _OtherIndexType>)
+ _LIBCPP_HIDE_FROM_ABI constexpr reference operator[](const array< _OtherIndexType, rank()>& __indices) const {
----------------
Needs a test. The `span` version has the same problem.
================
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);
+ }
----------------
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.
================
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);
+ }
----------------
Note to self: I have not actually reviewed the implementation of these `idx_fold` operations yet.
================
Comment at: libcxx/include/__mdspan/mdspan.h:130-131
+private:
+ // Can't use defaulted parameter in the __deduction_workaround template because of a bug in MSVC warning C4348.
+ using __impl = __deduction_workaround<std::make_index_sequence<extents_type::rank()>>;
+
----------------
philnik wrote:
> libc++ doesn't work with MSVC anyways, so this workaround can be removed.
Not sure what this comment refers to, but the nvhpc comment should be removed.
================
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};
+};
----------------
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.
================
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());
+
----------------
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154367/new/
https://reviews.llvm.org/D154367
More information about the libcxx-commits
mailing list