[libcxx-commits] [PATCH] D157171: [libc++] mdspan - implement layout_stride

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added subscribers: var-const, philnik.
philnik added inline comments.


================
Comment at: libcxx/docs/Status/Cxx23Papers.csv:94-96
 "`P2599R2 <https://wg21.link/P2599R2>`__","LWG","``mdspan::size_type`` should be ``index_type``","July 2022","",""
 "`P2604R0 <https://wg21.link/P2604R0>`__","LWG","mdspan: rename pointer and contiguous","July 2022","",""
 "`P2613R1 <https://wg21.link/P2613R1>`__","LWG","Add the missing ``empty`` to ``mdspan``","July 2022","",""
----------------
These should all be completed with this patch too.


================
Comment at: libcxx/include/__mdspan/layout_stride.h:148
+        }(make_index_sequence<__rank_>())) {
+    _LIBCPP_ASSERT(([&]<size_t... _Pos>(index_sequence<_Pos...>) {
+                     // For integrals we can do a pre-conversion check, for other types not
----------------
Throughout. @var-const Do you have any thoughts here?


================
Comment at: libcxx/include/__mdspan/layout_stride.h:163
+            // basically sort the dimensions based on strides and extents, sorting is represented in permute array
+            std::array<rank_type, __rank_> __permute{_Pos...};
+            for (rank_type __i = __rank_ - 1; __i > 0; __i--) {
----------------



================
Comment at: libcxx/include/__mdspan/layout_stride.h:227
+    _LIBCPP_ASSERT(([&]<size_t... _Pos>(index_sequence<_Pos...>) {
+                     return static_cast<index_type>(0) == static_cast<index_type>(__other((_Pos ? 0 : 0)...));
+                   }(make_index_sequence<__rank_>())),
----------------
Maybe?


================
Comment at: libcxx/include/__mdspan/layout_stride.h:267
+    // However, mdspan does check this on its own, so for now we avoid double checking in hardened mode
+    _LIBCPP_ASSERT(__mdspan_detail::__is_multidimensional_index_in(__extents_, __idx...),
+                   "layout_stride::mapping: out of bounds indexing");
----------------



================
Comment at: libcxx/include/__mdspan/layout_stride.h:290
+        if constexpr (__rank_ == 1)
+          return true;
+        else {
----------------
Test!


================
Comment at: libcxx/include/__mdspan/layout_stride.h:311
+  _LIBCPP_HIDE_FROM_ABI constexpr index_type stride(rank_type __r) const noexcept
+    requires(__rank_ > 0)
+  {
----------------
Where does this come from?


================
Comment at: libcxx/include/__mdspan/layout_stride.h:313
+  {
+    _LIBCPP_ASSERT(__r < __rank_, "layout_stride::mapping::stride(): invalid rank index");
+    return __strides_[__r];
----------------



================
Comment at: libcxx/test/libcxx/containers/views/mdspan/layout_stride/assert.conversion.pass.cpp:79
+    std::extents<int, D, D> arg_exts{100, 5};
+    std::layout_stride::mapping<std::extents<int, D, D>> arg(arg_exts, std::array<int, 2>{1,100});
+    // check extents would be constructible
----------------
Maybe try to trigger the edge case here? Just ignore the comment if it doesn't work.


================
Comment at: libcxx/test/libcxx/containers/views/mdspan/layout_stride/assert.conversion.pass.cpp:83
+    // but the product is not, so we can't use it for layout_stride
+    TEST_LIBCPP_ASSERT_FAILURE(
+        ([=] { std::layout_stride::template mapping<std::extents<char, D, 5>> m(arg); }()),
----------------
Possible future work: Add tests to make sure that `mdspan` and friends work with `_BitInt`.


================
Comment at: libcxx/test/libcxx/containers/views/mdspan/layout_stride/assert.ctor.extents_array.pass.cpp:52
+    // check that if we first overflow in strides conversion we also fail
+    assert(static_cast<unsigned char>(257u) == 1);
+    TEST_LIBCPP_ASSERT_FAILURE(
----------------



================
Comment at: libcxx/test/libcxx/containers/views/mdspan/layout_stride/assert.ctor.extents_span.pass.cpp:53
+    // check that if we first overflow in strides conversion we also fail
+    assert(static_cast<unsigned char>(257u) == 1);
+    TEST_LIBCPP_ASSERT_FAILURE(
----------------



================
Comment at: libcxx/test/libcxx/containers/views/mdspan/layout_stride/assert.ctor.extents_span.pass.cpp:82
+          std::layout_stride::template mapping<std::extents<unsigned, D, 5, 7>>
+            m(std::extents<unsigned, D, 5, 7>(20), std::array<unsigned, 3>{4, 1, 200});
+        }()),
----------------
This should be a span!


================
Comment at: libcxx/test/std/containers/views/mdspan/CustomTestLayouts.h:278
       size *= extents_.extent(r);
-    return size;
+    return std::max(size*scaling_ + offset_, offset_);
   }
----------------



================
Comment at: libcxx/test/std/containers/views/mdspan/layout_stride/comparison.pass.cpp:49-55
+constexpr X compare_layout_mappings(...) { return {}; }
+
+template <class E1, class E2>
+constexpr auto compare_layout_mappings(E1 e1, std::array<int, E1::rank()> s1, E2 e2, std::array<int, E2::rank()> s2)
+    -> decltype(std::layout_stride::mapping<E1>(e1, s1) == std::layout_stride::mapping<E2>(e2, s2)) {
+  return true;
+}
----------------
```lang=c++
template <class E1, class E2>
concept layout_mapping_comparble = requires(E1 e1, E2 e2) { std::layout_stride::mapping<E1>(e1, s1) == std::layout_stride::mapping<E2>(e2, s2); }
```
should also work, right?


================
Comment at: libcxx/test/std/containers/views/mdspan/layout_stride/ctor.default.pass.cpp:37
+  // check correct extents are returned
+  ASSERT_NOEXCEPT(m.extents());
+  assert(m.extents() == e);
----------------
Please add a test to make sure `extents()` is const qualified. Also for `required_span_size()` and `strides()`.


================
Comment at: libcxx/test/std/containers/views/mdspan/layout_stride/ctor.extents_array.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Note to self: Continue here.


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

https://reviews.llvm.org/D157171