SpaceAwareAccessor #7074

crtrott · 2024-06-17T05:28:25Z

This will be the default accessor for View adding memory access check.

crtrott · 2024-06-17T05:29:00Z

Needs a rebase after merging the thing with the default mdspan on.

dalg24 · 2024-06-17T07:54:39Z

core/unit_test/TestSpaceAwareAccessor.hpp

+  {
+    int errors;
+    using acc_t =
+        Kokkos::Impl::SpaceAwareAccessor<memory_space_t,
+                                         Kokkos::default_accessor<const T>>;
+    acc_t acc{};
+    typename acc_t::data_handle_type ptr = v.data();
+
+    Kokkos::parallel_reduce(
+        Kokkos::RangePolicy<ExecutionSpace>(0, v.extent(0)),
+        KOKKOS_LAMBDA(int i, int& error) {
+          if (acc.access(ptr, i) != ptr[i]) error++;
+          static_assert(std::is_same_v<typename acc_t::element_type, const T>);
+          static_assert(std::is_same_v<typename acc_t::reference, const T&>);
+          static_assert(
+              std::is_same_v<typename acc_t::data_handle_type, const T*>);
+          static_assert(std::is_same_v<typename acc_t::offset_policy, acc_t>);
+          static_assert(
+              std::is_same_v<typename acc_t::memory_space, memory_space_t>);
+          static_assert(std::is_empty_v<acc_t>);
+        },
+        errors);
+    ASSERT_EQ(errors, 0);
+  }


Don't you get this for free if you instantiate your test for a const-qualified type?
Please get rid of that block and instantiate for some const T.

I guess I can instantiate the View with remove_const

dalg24 · 2024-06-17T08:03:30Z

core/src/View/MDSpan/Kokkos_MDSpan_Accessor.hpp

+
+  KOKKOS_FUNCTION
+  constexpr reference access(data_handle_type p, size_t i) const noexcept {
+    // Same error as Kokkos view in case anyone greps for this in their tools


I don't understand your comment and unsure on what's best to do here (pretend it is a view error or say accessor)

Interesting question actually: originally I thought we just check error here, but that means no label. I guess we could internally use an mdspan without SpaceAwareAccessor and just use one with SpaceAwareAccessor in the public interfaces.

changed the message

dalg24 · 2024-06-17T08:33:22Z

core/src/View/MDSpan/Kokkos_MDSpan_Accessor.hpp

+  template <class, class>
+  friend struct SpaceAwareAccessor;
+};
+


Should we attempt to handle the case where NestedAccessor is a specialization of SpaceAwareAccessor in which case we may be able to "collapse" and not produce and double-nested accessor?

If and when we make this public maybe. For now I wouldn't say so since we are not gonna use it in generic contexts?

dalg24 · 2024-06-17T08:33:23Z

core/src/View/MDSpan/Kokkos_MDSpan_Accessor.hpp

+};
+
+template <class NestedAccessor>
+struct SpaceAwareAccessor<AnonymousSpace, NestedAccessor> {


I suppose since NestedAccessor can be some potentially weird user-defined type, it is not safe to just derive from it to save on the boiler plate code below?

yeah, not sure I prefer inheritance even if it worked, i mean there isn't that much stuff defined.

dalg24 · 2024-06-17T12:30:12Z

core/src/View/MDSpan/Kokkos_MDSpan_Accessor.hpp

+#include <Kokkos_Concepts.hpp>
+#include <impl/Kokkos_Utilities.hpp>


What are you using from these?
I think all you need is an include for Impl::runtime_check_memory_access_violation()

Macros: KOKKOS_FUNCTION
Concepts: is_memory_space
Utilities: runtime_check_memory_access_violation

or?

Suggested change

#include <Kokkos_Concepts.hpp>

#include <impl/Kokkos_Utilities.hpp>

#include <Kokkos_Core_fwd.hpp> // runtime_check_memory_access_violation

runtime_check_memory_access_violation (without ViewTracker and ViewMapping argument) is defined in Kokkos_Core_fwd.hpp (which also includes Kokkos_Macros.hpp).

dalg24 · 2024-06-17T12:42:28Z

cmake/kokkos_enable_options.cmake

@@ -76,7 +76,7 @@ KOKKOS_ENABLE_OPTION(IMPL_HIP_UNIFIED_MEMORY OFF "Whether to leverage unified me
 KOKKOS_ENABLE_OPTION(DESUL_ATOMICS_EXTERNAL OFF "Whether to use an external desul installation")
 KOKKOS_ENABLE_OPTION(ATOMICS_BYPASS OFF "**NOT RECOMMENDED** Whether to make atomics non-atomic for non-threaded MPI-only use cases")

-KOKKOS_ENABLE_OPTION(IMPL_MDSPAN OFF "Whether to enable experimental mdspan support")
+KOKKOS_ENABLE_OPTION(IMPL_MDSPAN ON "Whether to enable experimental mdspan support")


I suppose you refer to this change and the CI configuration ones when you talk about needing to rebase.
In any case commenting here to note this does not belong to this PR.

OK now I ran into #7069

rebase has happened

core/src/Kokkos_View.hpp

dalg24 · 2024-06-17T12:51:32Z

core/src/Kokkos_View.hpp

+      typename Traits::memory_space,
+      Kokkos::default_accessor<typename Traits::value_type>>;
+  using mdspan_type = mdspan<typename Traits::value_type, extents_type,
+                             mdspan_layout_type, accessor_type>;


Should we comment somewhere that the converting constructor from a mdspan strips out the accessor and just require convertibility to a space-aware default accessor?

not sure what you mean, the constructor takes this thing. So all the rules for mdspan conversion apply anyway. View will only do something with it after the user thing was converted to mdspan_type

dalg24 · 2024-06-17T12:56:02Z

core/unit_test/TestSpaceAwareAccessorAccessViolation.hpp

+}
+#endif
+
+TEST(TEST_CATEGORY_DEATH, memory_access_violations_from_device) {


We might want some sort of mdpan_ prefix to classify that test properly.

its not mdspan. I guess I can prefix it with space_aware_accessor making it space_aware_accessor_memory_access_violations_from_device ...

changed the name

This mdspan accessor wraps otehr accessors, and adds memory space accessibility checks.

That version is convertible into all other variants

CI serviced these. Also turn off tests if mdspan is not enabled

nmm0

Looks good, I think we just need to figure out the explicit conversion thing

nmm0 · 2024-06-17T19:57:13Z

core/src/View/MDSpan/Kokkos_MDSpan_Accessor.hpp

+            std::enable_if_t<std::is_constructible_v<NestedAccessor,
+                                                     OtherNestedAccessorType>,
+                             int> = 0>
+  KOKKOS_FUNCTION constexpr SpaceAwareAccessor(


At the very least I think this constructor should be explicit

We will probably discuss this either during the team meeting or a separate meeting

Yes I want to have a full discussion on how to consistently deal with explicitness for View and related facilities.

nmm0 · 2024-06-17T19:57:49Z

core/src/View/MDSpan/Kokkos_MDSpan_Accessor.hpp

+      : nested_acc(other.nested_acc) {}
+
+  KOKKOS_FUNCTION
+  SpaceAwareAccessor(const NestedAccessor& acc) : nested_acc(acc) {}


nmm0 · 2024-06-18T21:18:08Z

core/unit_test/TestSpaceAwareAccessor.hpp

+  Kokkos::parallel_reduce(
+      Kokkos::RangePolicy<ExecutionSpace>(0, v.extent(0)),
+      KOKKOS_LAMBDA(int i, int& error) {
+        if (acc.access(ptr, i) != ptr[i]) error++;


very nitpicky but we may want to make an using acc_base_t = Kokkos::default_accessor<T> and make this check acc.access(ptr,i) != acc_base.access(ptr,i) just to make the requirements clearer

I don't agree with that necessarily pretending this tests whether we really just call through seems at best misleading.

crtrott · 2024-06-18T21:49:56Z

core/src/View/MDSpan/Kokkos_MDSpan_Accessor.hpp

+#else
+  [[no_unique_address]]
+#endif
+  NestedAccessor nested_acc;


Because the question came up: yes you need no_unique_address recursively for it to work:
https://godbolt.org/z/T84E7Yb1f

masterleinad · 2024-06-19T13:39:40Z

core/src/View/MDSpan/Kokkos_MDSpan_Accessor.hpp

+#include <Kokkos_Concepts.hpp>
+#include <impl/Kokkos_Utilities.hpp>


Suggested change

#include <Kokkos_Concepts.hpp>

#include <impl/Kokkos_Utilities.hpp>

#include <Kokkos_Core_fwd.hpp> // runtime_check_memory_access_violation

runtime_check_memory_access_violation (without ViewTracker and ViewMapping argument) is defined in Kokkos_Core_fwd.hpp (which also includes Kokkos_Macros.hpp).

masterleinad · 2024-06-19T13:40:09Z

core/src/View/MDSpan/Kokkos_MDSpan_Accessor.hpp

+
+namespace Kokkos {
+
+// For now use the accessors in IMPL namespace, as an


Suggested change

// For now use the accessors in IMPL namespace, as an

// For now use the accessors in Impl namespace, as an

Kokkos_Core_fwd.hpp straight includes impl/Kokkos_Utilities.hpp but it doesn't include Kokkos_Concept.hpp so no reason to change that.

masterleinad · 2024-06-19T13:45:59Z

core/src/View/MDSpan/Kokkos_MDSpan_Accessor.hpp

+
+  using memory_space = MemorySpace;
+
+  static_assert(is_memory_space<memory_space>::value);


Suggested change

static_assert(is_memory_space<memory_space>::value);

static_assert(is_memory_space_v<memory_space>);

dalg24 reviewed Jun 17, 2024

View reviewed changes

crtrott force-pushed the space-aware-accessor branch from 00fcb08 to 165bcb4 Compare June 17, 2024 18:15

crtrott added 5 commits June 17, 2024 17:01

Add SpaceAwareAccessor

afbff6c

This mdspan accessor wraps otehr accessors, and adds memory space accessibility checks.

Add basic tests for SpaceAwareAccessor

b2046a4

Use SpaceAwareAccessor in View mdspan-interop

e2d68fd

Add specialization of SpaceAwareAcc for AnonymousSpace

a6b95e9

That version is convertible into all other variants

Address review comments

34db518

crtrott force-pushed the space-aware-accessor branch 2 times, most recently from fc4f684 to f29c746 Compare June 18, 2024 17:56

SpaceAwareAccessor: fix issues (no-unique-address, is_empty)

6c78f4b

CI serviced these. Also turn off tests if mdspan is not enabled

crtrott force-pushed the space-aware-accessor branch from f29c746 to 6c78f4b Compare June 18, 2024 19:45

nliber approved these changes Jun 18, 2024

View reviewed changes

nmm0 reviewed Jun 18, 2024

View reviewed changes

crtrott commented Jun 18, 2024

View reviewed changes

masterleinad reviewed Jun 19, 2024

View reviewed changes

Use correct includes for spaceawareaccessor

7b962ce

masterleinad approved these changes Jun 19, 2024

View reviewed changes

crtrott merged commit 362c9d7 into kokkos:develop Jun 19, 2024
28 of 29 checks passed

crtrott deleted the space-aware-accessor branch June 19, 2024 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SpaceAwareAccessor #7074

SpaceAwareAccessor #7074

crtrott commented Jun 17, 2024

crtrott commented Jun 17, 2024

dalg24 Jun 17, 2024

crtrott Jun 17, 2024

crtrott Jun 18, 2024

dalg24 Jun 17, 2024

crtrott Jun 17, 2024

crtrott Jun 18, 2024

dalg24 Jun 17, 2024

crtrott Jun 17, 2024

dalg24 Jun 17, 2024

crtrott Jun 18, 2024 •

edited

Loading

dalg24 Jun 17, 2024

crtrott Jun 17, 2024

masterleinad Jun 19, 2024

dalg24 Jun 17, 2024

dalg24 Jun 17, 2024

crtrott Jun 18, 2024

dalg24 Jun 17, 2024

crtrott Jun 17, 2024

dalg24 Jun 17, 2024

crtrott Jun 17, 2024

crtrott Jun 18, 2024

nmm0 left a comment

nmm0 Jun 17, 2024

nmm0 Jun 18, 2024

crtrott Jun 18, 2024

nmm0 Jun 17, 2024

nmm0 Jun 18, 2024

crtrott Jun 18, 2024

crtrott Jun 18, 2024

masterleinad Jun 19, 2024

masterleinad Jun 19, 2024

crtrott Jun 19, 2024

masterleinad Jun 19, 2024

		#include <Kokkos_Concepts.hpp>
		#include <impl/Kokkos_Utilities.hpp>

	#include <Kokkos_Concepts.hpp>
	#include <impl/Kokkos_Utilities.hpp>
	#include <Kokkos_Core_fwd.hpp> // runtime_check_memory_access_violation


		namespace Kokkos {

		// For now use the accessors in IMPL namespace, as an


		using memory_space = MemorySpace;

		static_assert(is_memory_space<memory_space>::value);

	static_assert(is_memory_space<memory_space>::value);
	static_assert(is_memory_space_v<memory_space>);

SpaceAwareAccessor #7074

SpaceAwareAccessor #7074

Conversation

crtrott commented Jun 17, 2024

crtrott commented Jun 17, 2024

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

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

Choose a reason for hiding this comment

crtrott Jun 18, 2024 • 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

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

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

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

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

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

Choose a reason for hiding this comment

crtrott Jun 18, 2024 •

edited

Loading