Add support for `View::rank[_dynamic]()` #5870

dalg24 · 2023-02-09T06:28:29Z

Related to #5717

Make View::rank[_dynamic] implicitly convertible to ~~int~~ size_t and callable
~~Deprecate View::Rank (never mentioned in the API reference for View)~~ done in Deprecate View::Rank "uppercase R" in favor of View::rank "lowercase r" #5882

~~Not sure it is worth the trouble. Opening so we can discuss.~~ We agreed to move forward at the dev meeting.

`View` member	Before this PR	After this PR
`View::Rank`	implementation-defined integral type	deprecated
`View::rank`	implementation-defined integral type	unspecified type that is convertible to `size_t`
`View::rank()`	not provided	returns `size_t`
`View::rank_dynamic`	implementation-defined integral type	unspecified type that is convertible to `size_t`
`View::rank_dynamic()`	not provided	returns `size_t`

Two things worth noting:

One advantage of the View::rank() member over the rank(View) free function is that it can be used in constant expressions.
The actual type of View::rank as it was defined before was left up to the implementation (up to the compiler not to Kokkos) but in practice it was often int which means this change may yield warnings about comparing signed and unsigned integral types. It may also break code that was using the type of View::rank. The rational for choosing size_t was to align with std::mdspan.

ldh4 · 2023-02-09T22:37:40Z

I agree with this change, keeping it consistent with ViewTraits, even though there seem to be handful of comparison of different integer signedness warnings to be handled.

We also happened to have called the struct that contains the iteration pattern Kokkos::Rank. So, renaming View::Rank to View::rank could help reducing potential source of that confusion, especially when used without fully qualified names.

PhilMiller · 2023-02-15T19:21:35Z

Down the line, whatever we do with these interfaces ought to support code that's generic over both View and DynRankView (and maybe indicate DynamicView as rank 1, because that's how it would be accessed and copied)

nmm0 · 2023-02-15T19:35:02Z

I added to the MDspan project since it's related (i.e. rank returning size_t)

nmm0 · 2023-02-16T22:23:31Z

core/src/impl/Kokkos_Utilities.hpp

+  using value_type         = T;
+  using type               = integral_constant<T, v>;
+  static constexpr T value = v;
+  KOKKOS_FUNCTION constexpr operator value_type() const noexcept {


If we're re-implementing it anyway because of the need for __host__ __device__ we could eventually deprecate this conversion if we wanted to make the view API more similar to mdspan.

That's assuming this utility class is not used elsewhere...

You read my mind

But not doing that for a while. We need to get users off of View::Rank first and we want to make it reasonably easy for them to support a range of Kokkos versions.

dalg24 · 2023-02-17T03:41:20Z

core/src/Kokkos_CopyViews.hpp

+      message += std::to_string(dst.extent(0));
+      for (size_t r = 1; r < dst_type::rank; r++) {


These changes are to avoid warning pointless comparison of unsigned integer with zero when the loop is from 0 to rank-1

dalg24 · 2023-02-17T03:43:01Z

core/src/Kokkos_View.hpp

@@ -814,14 +814,14 @@ class View : public ViewTraits<DataType, Properties...> {

  template <typename... Is>
  static KOKKOS_FUNCTION void check_access_member_function_valid_args(Is...) {
-    static_assert(traits::rank <= sizeof...(Is), "");
+    static_assert(rank <= sizeof...(Is), "");


traits::rank[_dynamic_rank] are still valid but it is cleaner without qualifying.

dalg24 · 2023-02-17T16:38:01Z

algorithms/src/std_algorithms/impl/Kokkos_Constraints.hpp

@@ -29,7 +29,7 @@ struct is_admissible_to_kokkos_std_algorithms : std::false_type {};

 template <typename T>
 struct is_admissible_to_kokkos_std_algorithms<
-    T, std::enable_if_t< ::Kokkos::is_view<T>::value && T::rank == 1 &&
+    T, std::enable_if_t< ::Kokkos::is_view<T>::value && T::rank() == 1 &&


MSVC was choking on that guy. Was giving failed static assertions all over the place in algorithms

dalg24 · 2023-02-17T16:38:55Z

core/src/Kokkos_View.hpp

@@ -1716,7 +1705,7 @@ struct RankDataType<ValueType, 0> {

 template <unsigned N, typename... Args>
 KOKKOS_FUNCTION std::enable_if_t<
-    N == View<Args...>::rank &&
+    N == View<Args...>::rank() &&


This change and the one below was for MSVC :(

Co-Authored-By: Christian Trott <crtrott@sandia.gov>

dalg24 · 2023-02-17T17:46:58Z

Retest this please

dalg24 · 2023-02-19T13:42:31Z

Retest this please

masterleinad

Looks OK to me.

dalg24 · 2023-02-22T17:14:59Z

Ignoring HIP failure

dalg24 mentioned this pull request Feb 9, 2023

Prefer Kokkos::View::{R->r}ank kokkos/kokkos-kernels#1679

Merged

nmm0 added this to In progress in Feature: MDSpan Feb 15, 2023

This was referenced Feb 16, 2023

Deprecate View::Rank "uppercase R" in favor of View::rank "lowercase r" #5880

Closed

Deprecate View::Rank "uppercase R" in favor of View::rank "lowercase r" #5882

Merged

dalg24 added 5 commits February 16, 2023 17:19

Add Impl::integral_constant

2e53f1c

Add View::rank[_dynamic] static constexpr data members

314b966

Fixup update Kokkos::rank(View) free function and drop outdated comment

10fae1f

Fix printing extents and rank in error message when copying views

a7daa59

Fix warning comparison of integers of different signs

4286774

dalg24 force-pushed the view_rank_member_function branch from 311d00f to 547ed20 Compare February 16, 2023 22:20

nmm0 reviewed Feb 16, 2023

View reviewed changes

dalg24 changed the title ~~Add support for View::rank[_dynamic]() and deprecate View::Rank~~ Add support for View::rank[_dynamic]() Feb 16, 2023

dalg24 force-pushed the view_rank_member_function branch from d5f33f0 to 6837770 Compare February 17, 2023 01:37

dalg24 commented Feb 17, 2023

View reviewed changes

dalg24 and others added 2 commits February 17, 2023 12:10

Cleanup prefer {traits:: -> }rank[_dynamic]

d3eac2b

Fix MSVC CI build

2969679

Co-Authored-By: Christian Trott <crtrott@sandia.gov>

dalg24 force-pushed the view_rank_member_function branch from 15bf1a3 to 2969679 Compare February 17, 2023 17:12

dalg24 marked this pull request as ready for review February 17, 2023 17:47

dalg24 added 3 commits February 17, 2023 13:32

Fix warning pointless comparison of unsigned integer with zero

25ff05b

Add compile-only test for View::rank[_dynamic]

3be7ae2

Merge branch 'develop' into view_rank_member_function

03aae9a

crtrott mentioned this pull request Feb 22, 2023

Fix MSVC/CUDA Build #5905

Merged

crtrott approved these changes Feb 22, 2023

View reviewed changes

masterleinad approved these changes Feb 22, 2023

View reviewed changes

dalg24 merged commit e146fc9 into kokkos:develop Feb 22, 2023

dalg24 deleted the view_rank_member_function branch February 22, 2023 17:15

dalg24 mentioned this pull request Feb 22, 2023

Update View::rank[_dynamic] API documentation for release 4.1 kokkos/kokkos-core-wiki#309

Merged

nmm0 moved this from In progress to Done in Feature: MDSpan Mar 8, 2023

ndellingwood mentioned this pull request Mar 24, 2023

intrepid2: compatibility update has_rank_member trilinos/Trilinos#11723

Merged

dalg24 mentioned this pull request Apr 3, 2023

CHANGELOG: 4.1.0 #5902

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `View::rank[_dynamic]()` #5870

Add support for `View::rank[_dynamic]()` #5870

dalg24 commented Feb 9, 2023 •

edited

ldh4 commented Feb 9, 2023

PhilMiller commented Feb 15, 2023

nmm0 commented Feb 15, 2023

nmm0 Feb 16, 2023

dalg24 Feb 16, 2023

dalg24 Feb 16, 2023

dalg24 Feb 17, 2023

dalg24 Feb 17, 2023

dalg24 Feb 17, 2023

dalg24 Feb 17, 2023

dalg24 commented Feb 17, 2023

dalg24 commented Feb 19, 2023

masterleinad left a comment

dalg24 commented Feb 22, 2023

		message += std::to_string(dst.extent(0));
		for (size_t r = 1; r < dst_type::rank; r++) {

Add support for View::rank[_dynamic]() #5870

Add support for View::rank[_dynamic]() #5870

Conversation

dalg24 commented Feb 9, 2023 • edited

ldh4 commented Feb 9, 2023

PhilMiller commented Feb 15, 2023

nmm0 commented Feb 15, 2023

nmm0 Feb 16, 2023

Choose a reason for hiding this comment

dalg24 Feb 16, 2023

Choose a reason for hiding this comment

dalg24 Feb 16, 2023

Choose a reason for hiding this comment

dalg24 Feb 17, 2023

Choose a reason for hiding this comment

dalg24 Feb 17, 2023

Choose a reason for hiding this comment

dalg24 Feb 17, 2023

Choose a reason for hiding this comment

dalg24 Feb 17, 2023

Choose a reason for hiding this comment

dalg24 commented Feb 17, 2023

dalg24 commented Feb 19, 2023

masterleinad left a comment

Choose a reason for hiding this comment

dalg24 commented Feb 22, 2023

Add support for `View::rank[_dynamic]()` #5870

Add support for `View::rank[_dynamic]()` #5870

dalg24 commented Feb 9, 2023 •

edited