New submdspan CPP 17 compatible #227

crtrott · 2023-01-06T21:31:12Z

No description provided.

This uses ADL customization point design

for strided_index_range.

crtrott · 2023-01-06T21:31:57Z

This is the equivalent to #223 just with C++17 support

mhoemmen · 2023-01-06T22:37:37Z

@youyu3 check this out

mhoemmen

EXCELLENT

Just one typo in a comment.

include/experimental/__p2630_bits/submdspan_extents.hpp

include/experimental/__p2630_bits/submdspan_mapping.hpp

mhoemmen · 2023-01-06T23:27:06Z

include/experimental/__p2630_bits/submdspan_extents.hpp

+  return get<1>(i);
+}
+
+// Suppress spurious warning with NVCC about no return statement.


I've seen this with NVCC + GCC 10. The GCC intrinsic function __builtin_unreachable(); has helped me get rid of the warnings in this case. C++23 adds std::unreachable, which hopefully will accomplish the same goal.

Does that GCC intrinsic also work with nvc++, clang, msvc as host compiler?

It's a GCC-only thing, so when I used it, I defined it as a macro.

GCC 10 isn't quite as smart as GCC 11 about constexpr auto functions that have exhaustive if constexpr branches in them.

Thanks for figuring this out! : - )

tests/test_submdspan.cpp

dalg24 · 2023-01-08T19:07:32Z

tests/test_submdspan.cpp

+  MDSPAN_INLINE_FUNCTION
+  static int create_slice_arg(int) {
+    return 2;
+  }
+  MDSPAN_INLINE_FUNCTION
+  static std::pair<int,int> create_slice_arg(std::pair<int,int>) {
+    return std::pair<int,int>(1,3);
+  }
+  MDSPAN_INLINE_FUNCTION
+  static stdex::strided_index_range<int,int,int> create_slice_arg(stdex::strided_index_range<int,int,int>) {
+    return stdex::strided_index_range<int,int,int>{1,3,2};
+  }
+  MDSPAN_INLINE_FUNCTION
+  static stdex::full_extent_t create_slice_arg(stdex::full_extent_t) {
+    return stdex::full_extent;
+  }
+
+  template<class SrcExtents, class SubExtents, class ... SliceArgs>
+  MDSPAN_INLINE_FUNCTION
+  static bool match_expected_extents(int src_idx, int sub_idx, SrcExtents src_ext, SubExtents sub_ext, int, SliceArgs ... slices) {
+    return match_expected_extents(++src_idx, sub_idx, src_ext, sub_ext, slices...);
+  }
+  template<class SrcExtents, class SubExtents, class ... SliceArgs>
+  MDSPAN_INLINE_FUNCTION
+  static bool match_expected_extents(int src_idx, int sub_idx, SrcExtents src_ext, SubExtents sub_ext, std::pair<int,int> p, SliceArgs ... slices) {
+    return (sub_ext.extent(sub_idx)==p.second-p.first) && match_expected_extents(++src_idx, ++sub_idx, src_ext, sub_ext, slices...);
+  }
+  template<class SrcExtents, class SubExtents, class ... SliceArgs>
+  MDSPAN_INLINE_FUNCTION
+  static bool match_expected_extents(int src_idx, int sub_idx, SrcExtents src_ext, SubExtents sub_ext, stdex::strided_index_range<int,int,int> p, SliceArgs ... slices) {
+    return (sub_ext.extent(sub_idx)==(p.extent+p.stride-1)/p.stride) && match_expected_extents(++src_idx, ++sub_idx, src_ext, sub_ext, slices...);
+  }
+  template<class SrcExtents, class SubExtents, class ... SliceArgs>
+  MDSPAN_INLINE_FUNCTION
+  static bool match_expected_extents(int src_idx, int sub_idx, SrcExtents src_ext, SubExtents sub_ext, stdex::full_extent_t, SliceArgs ... slices) {
+    return (sub_ext.extent(sub_idx)==src_ext.extent(src_idx)) && match_expected_extents(++src_idx, ++sub_idx, src_ext, sub_ext, slices...);
+  }
+  template<class SrcExtents, class SubExtents>
+  MDSPAN_INLINE_FUNCTION
+  static bool match_expected_extents(int, int, SrcExtents, SubExtents) { return true; }


I am wondering whether we are we being to clever. (I don't have a better idea.)

tests/test_submdspan.cpp

include/experimental/__p2630_bits/submdspan_mapping.hpp

dalg24 · 2023-01-08T19:56:11Z

include/experimental/__p2630_bits/submdspan_extents.hpp

+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//@HEADER
+


include headers as appropriate

include/experimental/__p2630_bits/submdspan_extents.hpp

mhoemmen · 2023-01-09T02:02:35Z

include/experimental/__p2630_bits/submdspan_extents.hpp

+          extents_constructor<K - 1, Extents, NewExtents..., dynamic_extent>;
+      return next_t::next_extent(
+          ext, slices...,
+          r.extent > 0 ? 1 + divide<index_t>(r.extent - 1, r.stride) : 0);


The code as it stands won't achieve the goal of divide, which is to return integral_constant if both inputs are integral_constant.

Adding 1 to integral_constant<size_t, Value> results in size_t, not integral_constant<size_t, Value+1>.

Even if adding 1 to integral_constant resulted in integral_constant, the result of the ternary operator would be common_type_t<integral_constant<size_t, Value>, int>, which is int.

https://godbolt.org/z/qWEdn5xM6

#include <utility> #include <type_traits> template<size_t Val1, size_t Val2> constexpr auto divide(std::integral_constant<size_t, Val1> x, std::integral_constant<size_t, Val2> y) { return std::integral_constant<size_t, Val1 / Val2>{}; } template<size_t Value> using Size = std::integral_constant<size_t, Value>; int main() { constexpr auto z = divide(Size<8>{}, Size<2>{}); static_assert(std::is_same_v<std::remove_const_t<decltype(z)>, Size<4>>); constexpr auto w = 1 + divide(Size<8>{}, Size<2>{}); static_assert(not std::is_same_v<std::remove_const_t<decltype(w)>, Size<4>>); static_assert(std::is_same_v<std::remove_const_t<decltype(w)>, size_t>); static_assert(not std::is_same_v<std::common_type_t<Size<4>, int>, Size<4>>); static_assert(not std::is_same_v<std::common_type_t<Size<4>, int>, size_t>); static_assert(std::is_same_v<std::common_type_t<Size<4>, int>, int>); return 0; }

This doesn't matter since it doesn't determine the type. Note the values I am generating here are used as constructor arguments for an extents type which is independently determined. In case static extents are generated or preserved, these values will at best be checked against the static_extents in the extents constructor.

integral_constant<T, Value> implicitly converts to T. This means that 1 + integral_constant<size_t, X>{} / integral_constant<size_t, Y>{} just returns 1 + X / Y, so you don't even need divide here.

https://godbolt.org/z/x48bnaW8e

include/experimental/__p2630_bits/submdspan_extents.hpp

nmm0

Looks good to me, just a few minor changes

nmm0 · 2023-01-14T15:43:58Z

include/experimental/__p2630_bits/submdspan_extents.hpp

+  )
+  MDSPAN_INLINE_FUNCTION
+  constexpr static auto next_extent(const Extents &ext, const Slice &sl,
+                                    SliceSpecifiers... slices) {


More of a code readability issue -- the slices and new extent parameters here are kind of mixed up into the same parameter pack so it's difficult to understand the code. Might be worth separating out the new extents into a separate parameter.

nmm0 · 2023-01-14T15:49:46Z

include/experimental/__p2630_bits/submdspan_extents.hpp

+                      const integral_constant<T1, v1> &) {
+  // cutting short division by zero
+  // this is used for strided_index_range with zero extent/stride
+  return integral_constant<IndexT, v0 == 0 ? 0 : v0 / v1>();


I don't really understand this -- should stride be allowed to be zero? I guess if it is not specified it will be zero but then I think it would make sense to transform the value to nonzero before we get here.

crtrott · 2023-01-24T20:56:29Z

I checked and this works now over my test suite of compilers/standards.

mhoemmen

I think this is fine for now. We can fix the strided_slice name and add Mandates checks later. Thanks!

mhoemmen · 2023-01-24T21:06:30Z

include/experimental/__p2630_bits/strided_index_range.hpp

+
+// Slice Specifier allowing for strides and compile time extent
+template <class OffsetType, class ExtentType, class StrideType>
+struct strided_index_range {


This is called strided_slice now in the paper, but I'm OK with doing the rename as a separate PR.

mhoemmen · 2023-01-24T21:10:14Z

include/experimental/__p2630_bits/strided_index_range.hpp

+  OffsetType offset;
+  ExtentType extent;
+  StrideType stride;
+};


Should we consider enforcing the Mandates here?

Suggested change

};

static_assert(is_integral_v<OffsetType> || is_integral_constant_v<OffsetType>);

static_assert(is_integral_v<ExtentType> || is_integral_constant_v<ExtentType>);

static_assert(is_integral_v<StrideType> || is_integral_constant_v<StrideType>);

};

In the change above, is_integral_constant_v is defined as follows.

template<class T> struct is_integral_constant : std::false_type {}; template<std::integral T, T Value> struct is_integral_constant<std::integral_constant<T, Value>> : std::true_type {}; template<class T> constexpr inline bool is_integral_constant_v = is_integral_constant<T>::value;

mhoemmen · 2023-01-24T21:20:51Z

include/experimental/__p2630_bits/submdspan_extents.hpp

+  return get<1>(i);
+}
+
+// Suppress spurious warning with NVCC about no return statement.


Thanks for figuring this out! : - )

…gles kokkos: fix reversed triangles in `trmm`, `trsm` and `trsv`

crtrott added 15 commits December 29, 2022 13:24

Added new submdspan implementation

e004a42

This uses ADL customization point design

Fix build errors on Mac

8cfc22f

Add missing strides parameter

9c97013

Account for strided_index_range stride in submdspan

4bf8a09

Fix bug in submdspan_extents for strided_index_range slice

4defa5e

Fix missing include

39450c1

Expand submdspan typed test to actually do runtime check

aefb6e5

Add simple strided_index_range test

f516e85

Clang 14 needed explicit structured binding construction

ce9c272

for strided_index_range.

Fix some comments and rename map to mapping in mapping_offset

f558537

Rewrite the logic to preserve layout_right and layout_left

8a4586b

Add more tests

79c1232

Reformatting submdspan_extents

55c9979

Apply clang-format to submdspan files

4749d80

Backport new submdspan to C++17

c8e33dc

Make new submdspan work with NVCC and test on device

8ff29f8

crtrott marked this pull request as ready for review January 6, 2023 22:35

crtrott added 3 commits January 6, 2023 16:05

Disable submdspan in C++14 mode

fa91897

Delete old submdspan impl

36b9376

Update license files where missing

471ecbc

mhoemmen approved these changes Jan 6, 2023

View reviewed changes

crtrott added 3 commits January 6, 2023 16:48

Based on Marks Review: always use C++17 inv_map_rank

d7f2b3e

Fix a comment

a08fc05

Test submdspan customization point

277bb26

dalg24 reviewed Jan 8, 2023

View reviewed changes

mhoemmen reviewed Jan 9, 2023

View reviewed changes

crtrott added 2 commits January 8, 2023 19:41

Apply some changes suggested in review.

7b80df4

Support and add test for compile time zero length/stride index range

15899dc

crtrott force-pushed the new-submdspan-cpp17 branch from 3d40324 to 15899dc Compare January 9, 2023 03:04

nmm0 approved these changes Jan 14, 2023

View reviewed changes

crtrott added 3 commits January 23, 2023 17:37

Fix some warnings and rename parameter pack

98c6086

Actually run the submdspan test on device too

b4a7aeb

Fix warnings

c99e6e5

mhoemmen approved these changes Jan 24, 2023

View reviewed changes

crtrott merged commit 5bbee18 into kokkos:stable Jan 24, 2023

crtrott deleted the new-submdspan-cpp17 branch January 24, 2023 21:57

mhoemmen pushed a commit to mhoemmen/mdspan that referenced this pull request Jul 26, 2023

Merge pull request kokkos#227 from NexGenAnalytics/fix-reversed-trian…

3a85f8c

…gles kokkos: fix reversed triangles in `trmm`, `trsm` and `trsv`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New submdspan CPP 17 compatible #227

New submdspan CPP 17 compatible #227

crtrott commented Jan 6, 2023

crtrott commented Jan 6, 2023

mhoemmen commented Jan 6, 2023

mhoemmen left a comment

mhoemmen Jan 6, 2023

crtrott Jan 6, 2023

mhoemmen Jan 6, 2023

mhoemmen Jan 6, 2023 •

edited

Loading

mhoemmen Jan 24, 2023

dalg24 Jan 8, 2023

dalg24 Jan 8, 2023

mhoemmen Jan 9, 2023 •

edited

Loading

crtrott Jan 9, 2023

mhoemmen Jan 9, 2023

nmm0 left a comment

nmm0 Jan 14, 2023

nmm0 Jan 14, 2023

crtrott commented Jan 24, 2023

mhoemmen left a comment

mhoemmen Jan 24, 2023

mhoemmen Jan 24, 2023

mhoemmen Jan 24, 2023

-};
+  static_assert(is_integral_v<OffsetType> || is_integral_constant_v<OffsetType>);
+  static_assert(is_integral_v<ExtentType> || is_integral_constant_v<ExtentType>);
+  static_assert(is_integral_v<StrideType> || is_integral_constant_v<StrideType>);
+};

New submdspan CPP 17 compatible #227

New submdspan CPP 17 compatible #227

Conversation

crtrott commented Jan 6, 2023

crtrott commented Jan 6, 2023

mhoemmen commented Jan 6, 2023

mhoemmen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhoemmen Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhoemmen Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmm0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crtrott commented Jan 24, 2023

mhoemmen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhoemmen Jan 6, 2023 •

edited

Loading

mhoemmen Jan 9, 2023 •

edited

Loading