Fix standard compliance #285

crtrott · 2023-10-20T20:14:42Z

This fixes most everything identified in the llvm tests which is wrong with our reference implementation, and where we haven't backported fixes yet from when we contributed our implementation to LLVM.

I haven't done mdspan yet.

The initalization change for the array was necessary to avoid an issue with unintitialized members of std:;array for libcxx with length zero.

crtrott · 2023-10-20T20:15:53Z

Outstanding FIXMEs:

../../tests/libcxx-backports/layout_left/layout_left.static_requirements.pass.cpp:  // static_assert(std::__mdspan_detail::__is_extents_v<E>); // FIXME
../../tests/libcxx-backports/layout_right/layout_right.static_requirements.pass.cpp:  //static_assert(std::__mdspan_detail::__is_extents_v<E>); //FIXME
../../tests/libcxx-backports/layout_stride/CMakeLists.txt:#mdspan_add_test(layout_stride.extents.verify) //FIXME
../../tests/libcxx-backports/layout_stride/layout_stride.ctor.strided_mapping.pass.cpp:  //test_layout<always_convertible_layout>(); //FIXME
../../tests/libcxx-backports/layout_stride/layout_stride.static_requirements.pass.cpp:  //static_assert(std::__mdspan_detail::__is_extents_v<E>); //FIXME
../../tests/libcxx-backports/mdspan/CMakeLists.txt:#mdspan_add_test(mdspan.conversion.pass) //FIXME
../../tests/libcxx-backports/mdspan/CMakeLists.txt:#mdspan_add_test(mdspan.index_operator.pass) //FIXME
../../tests/libcxx-backports/mdspan/CMakeLists.txt:#mdspan_add_test(mdspan.swap.pass) //FIXME
../../tests/libcxx-backports/mdspan/mdspan.conversion.pass.cpp:  // FIXME: these tests trigger what appears to be a compiler bug on MINGW32 with --target=x86_64-w64-windows-gnu
../../tests/libcxx-backports/mdspan/mdspan.ctor.copy.pass.cpp:  //static_assert(noexcept(MDS(m_org)) == (noexcept(H(handle))&& noexcept(M(map))&& noexcept(A(acc)))); //FIXME
../../tests/libcxx-backports/mdspan/mdspan.ctor.default.pass.cpp:    //static_assert(noexcept(MDS()) == (noexcept(H())&& noexcept(M())&& noexcept(A()))); //FIXME
../../tests/libcxx-backports/mdspan/mdspan.ctor.default.pass.cpp:  // static_assert(test()); //FIXME
../../tests/libcxx-backports/mdspan/mdspan.ctor.dh_extents.pass.cpp:    //static_assert(!std::is_constructible_v<MDS, const H&, const typename M::extents_type&>); //FIXME
../../tests/libcxx-backports/mdspan/mdspan.ctor.dh_span.pass.cpp:  //test_mdspan_ctor<mec, ac>(handle, construct_mapping(layout, std::extents<int>()), acc); //FIXME
../../tests/libcxx-backports/mdspan/mdspan.ctor.dh_span.pass.cpp:  //test_mdspan_ctor<mec, ac>(handle, construct_mapping(layout, std::extents<unsigned, 7>()), acc); //FIXME
../../tests/libcxx-backports/mdspan/mdspan.deduction.pass.cpp:  //ASSERT_SAME_TYPE(decltype(std::mdspan(handle, 5, SizeTIntType(6))), std::mdspan<T, std::dextents<size_t, 2>>); //FIXME
../../tests/libcxx-backports/mdspan/mdspan.properties.pass.cpp:  //ASSERT_SAME_TYPE(decltype(m.size()), typename MDS::size_type); //FIXME
../../tests/libcxx-backports/mdspan/mdspan.properties.pass.cpp:  //assert(!noexcept(MDS::is_always_unique())); //FIXME
../../tests/libcxx-backports/mdspan/mdspan.properties.pass.cpp:  //assert(!noexcept(MDS::is_always_exhaustive())); //FIXME
../../tests/libcxx-backports/mdspan/mdspan.properties.pass.cpp:  //assert(!noexcept(MDS::is_always_strided())); //FIXME
../../tests/libcxx-backports/mdspan/mdspan.properties.pass.cpp:  //assert(!noexcept(m.is_unique())); //FIXME
../../tests/libcxx-backports/mdspan/mdspan.properties.pass.cpp:  //assert(!noexcept(m.is_exhaustive())); //FIXME
../../tests/libcxx-backports/mdspan/mdspan.properties.pass.cpp:  //assert(!noexcept(m.is_strided())); //FIXME
../../tests/libcxx-backports/mdspan/mdspan.properties.pass.cpp:  //static_assert(test()); //FIXME

mhoemmen · 2023-10-20T21:36:33Z

include/experimental/__p0009_bits/extents.hpp

+      for (size_type r = 0; r < m_rank; r++)
+        if(rhs.extent(r) != lhs.extent(r)) return false;


Suggested change

for (size_type r = 0; r < m_rank; r++)

if(rhs.extent(r) != lhs.extent(r)) return false;

for (size_type r = 0; r < m_rank; r++) {

if (rhs.extent(r) != lhs.extent(r)) {

return false;

}

}

Proposing to run the whole thing through clang-format and starting to enforce it.

include/experimental/__p0009_bits/layout_right.hpp

mhoemmen

The one lambda with explicit template parameters [&] <size_t...> { /* ... */ } requires C++20, but it's not protected with a macro. Otherwise, this is great! : - )

include/experimental/__p0009_bits/layout_stride.hpp

Fix formatting

include/experimental/__p0009_bits/layout_stride.hpp

mhoemmen

AWESOME

crtrott · 2023-10-26T19:46:47Z

compilation_tests/ctest_layout_convertible.cpp

 MDSPAN_STATIC_TEST(
  std::is_constructible<LS2, AStridedLayout<true>::mapping<E1>>::value &&
-  std::is_convertible<AStridedLayout<true>::mapping<E1>, LS2>::value
+  !std::is_convertible<AStridedLayout<true>::mapping<E1>, LS2>::value


Only std:: layouts are convertible, everything else is explicit in the standard.

crtrott · 2023-10-26T19:47:40Z

include/experimental/__p0009_bits/extents.hpp

 #ifdef __cpp_lib_span
  MDSPAN_TEMPLATE_REQUIRES(class T, size_t N,
-                           /* requires */ (N == m_size_dynamic))
+                           /* requires */ (N == m_size_dynamic && N > 0))


needed a separate constructor to make this work warning free, and make it work in constant expressions. Same strategy as with the from std::array ctor.

crtrott · 2023-10-26T19:47:51Z

include/experimental/__p0009_bits/extents.hpp

      /* requires */
      (
-          _MDSPAN_TRAIT(std::is_convertible, OtherIndexType, index_type) &&
+          _MDSPAN_TRAIT(std::is_convertible, const OtherIndexType&, index_type) &&


This is what the standard says.

Same below. The LLVM tests catch the difference.

crtrott · 2023-10-26T19:48:38Z

include/experimental/__p0009_bits/extents.hpp

 extents(IndexTypes...)
    -> extents<size_t,
-               size_t((IndexTypes(), ::MDSPAN_IMPL_STANDARD_NAMESPACE::dynamic_extent))...>;
+               ((void) sizeof(IndexTypes), ::MDSPAN_IMPL_STANDARD_NAMESPACE::dynamic_extent)...>;


Don't require default constructibility of IndexTypes to make the deduction guide work.

crtrott · 2023-10-26T19:49:10Z

include/experimental/__p0009_bits/layout_left.hpp

-       for(rank_type r=0; r<__extents.rank(); r++) {
-         if(stride != static_cast<index_type>(other.stride(r))) {
-           // Note this throw will lead to a terminate if triggered since this function is marked noexcept
-           throw std::runtime_error("Assigning layout_stride to layout_left with invalid strides.");


complains about throw in noexcept on various compilers (warning).

crtrott · 2023-10-26T19:49:23Z

include/experimental/__p0009_bits/layout_left.hpp

-    MDSPAN_INLINE_FUNCTION constexpr bool is_strided() const noexcept { return true; }
+    MDSPAN_INLINE_FUNCTION static constexpr bool is_unique() noexcept { return true; }
+    MDSPAN_INLINE_FUNCTION static constexpr bool is_exhaustive() noexcept { return true; }
+    MDSPAN_INLINE_FUNCTION static constexpr bool is_strided() noexcept { return true; }


This is what the standard says.

crtrott · 2023-10-26T19:49:42Z

include/experimental/__p0009_bits/layout_left.hpp

+    MDSPAN_TEMPLATE_REQUIRES(
+      class OtherExtents,
+      /* requires */ ( Extents::rank() == OtherExtents::rank())
+    )


constraint in the standard.

crtrott · 2023-10-26T19:49:50Z

include/experimental/__p0009_bits/layout_left.hpp

+      class OtherExtents,
+      /* requires */ ( Extents::rank() == OtherExtents::rank())
+    )
    MDSPAN_INLINE_FUNCTION


constraint in the standard.

crtrott · 2023-10-26T19:50:06Z

include/experimental/__p0009_bits/layout_right.hpp

-       for(rank_type r=__extents.rank(); r>0; r--) {
-         if(stride != static_cast<index_type>(other.stride(r-1))) {
-           // Note this throw will lead to a terminate if triggered since this function is marked noexcept
-           throw std::runtime_error("Assigning layout_stride to layout_right with invalid strides.");


get rid of warning and make it work in constant expression

crtrott · 2023-10-26T19:50:17Z

include/experimental/__p0009_bits/layout_right.hpp

-    MDSPAN_INLINE_FUNCTION constexpr bool is_strided() const noexcept { return true; }
+    MDSPAN_INLINE_FUNCTION static constexpr bool is_unique() noexcept { return true; }
+    MDSPAN_INLINE_FUNCTION static constexpr bool is_exhaustive() noexcept { return true; }
+    MDSPAN_INLINE_FUNCTION static constexpr bool is_strided() noexcept { return true; }


standard says so

crtrott · 2023-10-26T19:50:56Z

include/experimental/__p0009_bits/layout_stride.hpp

    //----------------------------------------------------------------------------

-    using __strides_storage_t = std::array<index_type, extents_type::rank()>;
+    using __strides_storage_t = detail::possibly_empty_array<index_type, extents_type::rank()>;


get rid of use of std::array, for GPUs etc. actually this is what the LLVM implementation does too, will make the class empty for rank-0 with clang (where std:;array actually contains a char).

crtrott · 2023-10-26T19:51:20Z

include/experimental/__p0009_bits/layout_stride.hpp

-               && _MDSPAN_FOLD_AND((self.extents().extent(Idxs) == other.extents().extent(Idxs)) /* || ... */);
+        using common_t = std::common_type_t<index_type, typename OtherExtents::index_type>;
+        return    _MDSPAN_FOLD_AND((static_cast<common_t>(self.stride(Idxs)) == static_cast<common_t>(other.stride(Idxs))) /* && ... */)
+               && _MDSPAN_FOLD_AND((static_cast<common_t>(self.extents().extent(Idxs)) == static_cast<common_t>(other.extents().extent(Idxs))) /* || ... */);


these and others like it eliminate warning for different signedness.

crtrott · 2023-10-26T19:51:50Z

include/experimental/__p0009_bits/layout_stride.hpp

+      MDSPAN_INLINE_FUNCTION
+      static constexpr std::array<index_type, extents_type::rank()> return_strides(const __strides_storage_t& s) {
+        return std::array<index_type, extents_type::rank()>{s[Idxs]...};
+      }


need this becasue we don't store a std::array for strides anymore.

crtrott · 2023-10-26T19:52:21Z

include/experimental/__p0009_bits/layout_stride.hpp

+      : __members{
+#else
+      : __base_t(__base_t{__member_pair_t(
+#endif


Need to create "layout_right" strides in default construction.

crtrott · 2023-10-26T19:52:39Z

include/experimental/__p0009_bits/layout_stride.hpp

+      !(std::is_convertible<typename StridedLayoutMapping::extents_type, extents_type>::value &&
+       (detail::__is_mapping_of<layout_left, StridedLayoutMapping> ||
+        detail::__is_mapping_of<layout_right, StridedLayoutMapping> ||
+        detail::__is_mapping_of<layout_stride, StridedLayoutMapping>))


this was just wrong.

LLVM tests catch this.

crtrott · 2023-10-26T19:53:07Z

include/experimental/__p0009_bits/layout_stride.hpp

+        } else {
+          return required_span_size() == __get_size(extents(), std::make_index_sequence<extents_type::rank()>());
+        }
+      }


Now fully implement corner case required_span_size.

crtrott · 2023-10-26T19:53:39Z

include/experimental/__p0009_bits/layout_stride.hpp

+        using common_t = std::common_type_t<index_type, typename StridedLayoutMapping::index_type>;
+        for(rank_type r = 0; r < extents_type::rank(); r++)
+          strides_match = strides_match && (static_cast<common_t>(x.stride(r)) == static_cast<common_t>(y.stride(r)));
+      }


get rid of warning

crtrott · 2023-10-26T19:54:01Z

include/experimental/__p0009_bits/mdspan.hpp

-      _MDSPAN_TRAIT(std::is_convertible, SizeType, index_type) &&
-      _MDSPAN_TRAIT(std::is_nothrow_constructible, index_type, SizeType) &&
+      _MDSPAN_TRAIT(std::is_convertible, const SizeType&, index_type) &&
+      _MDSPAN_TRAIT(std::is_nothrow_constructible, index_type, const SizeType&) &&


this is what the standard actually says.

crtrott added 14 commits October 20, 2023 12:08

Fix missing ctor in extents for length 0 span

2c6ef69

Fix extents comparison with different rank

43a0565

Fix layout comparison for different rank

5294c2e

Fix extents CTAD for non-default-constructible index type

4069fbe

Fix constructibility corner case of extents from span/array

bc2fabd

Fix corner case constructibility issue of mdspan

f3f451d

Fix more corner case conversion stuff

4296043

The initalization change for the array was necessary to avoid an issue with unintitialized members of std:;array for libcxx with length zero.

Fix static-ness of layout_left/right properties

bb93aaa

Enable another fixed layout_right test

70911ca

Fix default construction of layout_stride

f37d0cc

Enable two more layout_stride tests which are fixed

c58614c

Fix layout_stride::is_exhaustive() corner case

0dd03f4

Enable some layout_stride property tests which are already fixed

9ff2207

Make layout_stride::stride() constraints standard compliant

1927de0

mhoemmen reviewed Oct 20, 2023

View reviewed changes

include/experimental/__p0009_bits/layout_right.hpp Outdated Show resolved Hide resolved

mhoemmen requested changes Oct 20, 2023

View reviewed changes

crtrott and others added 4 commits October 23, 2023 11:12

Fix more warnings in exposed by the llvm tests

12cb47d

Guard C++23 consteval usage in libcxx tests

5947d31

Update include/experimental/__p0009_bits/layout_stride.hpp

b9e0120

Fix formatting

Fix formatting

71cfc20

mhoemmen reviewed Oct 23, 2023

View reviewed changes

include/experimental/__p0009_bits/layout_stride.hpp Outdated Show resolved Hide resolved

Fix formatting

4b94330

mhoemmen approved these changes Oct 23, 2023

View reviewed changes

Fix layout_stride convertibility

8328f29

crtrott force-pushed the fix-standard-compliance branch from bcdb5e0 to 8328f29 Compare October 23, 2023 19:02

crtrott commented Oct 26, 2023

View reviewed changes

crtrott merged commit b186529 into kokkos:stable Oct 26, 2023

This was referenced Jun 12, 2025

Avoid TU-local entities #413

Merged

Add GitHub clang-format-16 check and indent accordingly #414

Open

		for (size_type r = 0; r < m_rank; r++)
		if(rhs.extent(r) != lhs.extent(r)) return false;

Fix standard compliance #285

Fix standard compliance #285

Uh oh!

Conversation

crtrott commented Oct 20, 2023

Uh oh!

crtrott commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhoemmen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhoemmen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

crtrott commented Oct 20, 2023 •

edited

Loading