Use Kokkos::view_alloc with Kokkos::resize/realloc and Kokkos::create_mirror* for Kokkos 3.7 #749

masterleinad · 2022-09-21T17:29:41Z

Fixes #641. Only files in src are changed.

aprokop · 2022-09-21T17:46:37Z

I really dislike how view_alloc just spread all over the place. But I suppose this is the only way to do it.

masterleinad · 2022-09-22T17:04:16Z

Retest this please.

PhilMiller · 2022-09-22T21:52:10Z

src/ArborX_DistributedTree.hpp

+#else
+#if KOKKOS_VERSION >= 30700
+  auto boxes_host =
+      Kokkos::create_mirror_view(Kokkos::view_alloc(space), boxes);
 #else


Does this need a space.fence()? I think not, but I'm not entirely confident

masterleinad · 2022-10-14T12:56:38Z

Retest this please.

test/tstKokkosToolsAnnotations.cpp

dalg24

I would slightly prefer if we waited for our requiring Kokkos 3.7

src/details/ArborX_DetailsUtils.hpp

dalg24 · 2022-10-14T14:09:53Z

src/details/ArborX_DetailsUtils.hpp

+  static_assert(Kokkos::is_execution_space<ExecutionSpace>::value);
+  static_assert(Kokkos::is_memory_space<MemorySpace>::value);


Both have an _v version (not requesting you to change)

aprokop · 2023-03-03T21:50:28Z

This will need to be rebased on #834. It also has to be reconciled with #833. I propose not doing changes related to create_layout_right_mirror* here, instead doing them in #833. The rest of the PR should be straightforward.

masterleinad · 2023-03-06T18:58:49Z

This will need to be rebased on #834. It also has to be reconciled with #833. I propose not doing changes related to create_layout_right_mirror* here, instead doing them in #833. The rest of the PR should be straightforward.

I'm happy to rebase this on top of #833 and #834 but I still think that we should also address create_layout_right_mirror here.

aprokop

Partial review

aprokop · 2023-03-07T22:32:16Z

scripts/check_format_cpp.sh

-if [ "${clang_format_major_version}" -ne 14 ] || [ "${clang_format_minor_version}" -ne 0 ] || [ "${clang_format_patch_version}" -ne 0 ]; then
-  echo "***   ArborX requires clang-format version 14.0.0,"
-  echo "***   but version ${clang_format_major_version}.${clang_format_minor_version}.${clang_format_patch_version} was found instead."
-  exit 1
-fi


Why remove this?

aprokop · 2023-03-07T22:32:52Z

src/details/ArborX_DetailsCrsGraphWrapperImpl.hpp

@@ -236,7 +236,7 @@ void queryImpl(ExecutionSpace const &space, Tree const &tree,
    // Exit early if either no results were found for any of the queries, or
    // nothing was inserted inside a callback for found results. This check
    // guarantees that the second pass will not be executed.
-    Kokkos::resize(out, 0);
+    Kokkos::resize(Kokkos::view_alloc(space), out, 0);


Do we need the space if we resize to 0?

aprokop · 2023-03-07T22:33:50Z

src/details/ArborX_DetailsDistributedTreeImpl.hpp

-    auto tmp_view =
-        Kokkos::create_mirror_view_and_copy(space, imports_layout_right);
+    auto tmp_view = Kokkos::create_mirror_view_and_copy(
+        Kokkos::view_alloc(space, typename ExecutionSpace::memory_space{}),


Does it need WithoutInitializing, or is that done by default?

That's done by default.

aprokop · 2023-03-07T22:34:21Z

src/ArborX_DistributedTree.hpp

@@ -184,7 +188,9 @@ DistributedTree<MemorySpace, Enable>::DistributedTree(
                         "ArborX::DistributedTree::"
                         "leave_count_in_local_trees"),
      comm_size);
-  auto bottom_tree_sizes_host = Kokkos::create_mirror_view(_bottom_tree_sizes);
+  auto bottom_tree_sizes_host = Kokkos::create_mirror_view(
+      Kokkos::view_alloc(host_exec), _bottom_tree_sizes);


WithoutInitializing?

aprokop · 2023-03-07T22:34:40Z

src/ArborX_DistributedTree.hpp

@@ -163,7 +165,9 @@ DistributedTree<MemorySpace, Enable>::DistributedTree(
                static_cast<void *>(boxes.data()), sizeof(Box), MPI_BYTE,
                getComm());
 #else
-  auto boxes_host = Kokkos::create_mirror_view(boxes);
+  auto boxes_host =
+      Kokkos::create_mirror_view(Kokkos::view_alloc(host_exec), boxes);


WithoutInitializing

aprokop · 2023-03-07T22:39:10Z

src/details/ArborX_DetailsDistributor.hpp

@@ -103,11 +103,16 @@ determineBufferLayout(ExecutionSpace const &space, InputView batched_ranks,
  auto restricted_unique_ranks = InputView(
      compact_ranks, std::make_pair(0, static_cast<int>(n_unique_ranks)));

-  auto const unique_ranks_host = Kokkos::create_mirror_view_and_copy(


Is there a version of create_mirror_view_and_copy that does that without intialization? It seems a bit weird to split this function into create_mirror_view and deep_copy just so that it does not initialize.

aprokop · 2023-03-07T22:41:37Z

src/details/ArborX_DetailsDistributor.hpp

-  auto device_permutation_indices =
-      Kokkos::create_mirror_view(DeviceType(), permutation_indices);
+  auto device_permutation_indices = Kokkos::create_mirror_view(
+      Kokkos::view_alloc(space, typename DeviceType::memory_space{}),


I think this can be WithoutInitializing.

aprokop · 2023-03-07T22:42:32Z

src/details/ArborX_DetailsDistributor.hpp

@@ -299,13 +305,13 @@ class Distributor

      dest_buffer_mirror =
          ArborX::Details::create_layout_right_mirror_view_and_copy(
-              typename ImportView::memory_space(), dest_buffer);
+              space, typename ImportView::memory_space(), dest_buffer);


Should not initialize, check.

create_layout_right_mirror_view_and_copy doesn't initialize anyway.

aprokop · 2023-03-07T22:45:11Z

I think there are too many changes in this single PR, and would prefer to split it.

masterleinad · 2023-03-07T22:52:39Z

I'm still waiting for #833.

aprokop · 2023-03-07T22:54:49Z

I'm still waiting for #833.

Ah, OK. I did not understand that. So we want to merge #833 in some form first?

masterleinad · 2023-03-07T23:07:27Z

Ah, OK. I did not understand that. So we want to merge #833 in some form first?

Yes, I was pretty much good with it and would just suggest #833 (comment) to be addressed. After that, I would incorporate the changes here, possibly also addressing #833 (comment).
We can of course discuss splitting this further.

masterleinad · 2023-03-08T03:53:29Z

Retest this please.

…_mirror*

aprokop

Partial review. I haven't looked at the distributed changes yet, or at the changes to add a memory space to creating right layout views. I have to understand why those are necessary.

aprokop · 2023-03-16T17:37:06Z

.gitlab-ci.yml

@@ -26,11 +26,11 @@ stages:
  tags:
    - nobatch

-.BuildKokkos:
+BuildKokkos:


I think it's fine to leave it open, so that it rebuilds every time.

aprokop · 2023-03-16T17:41:23Z

src/details/ArborX_MinimumSpanningTree.hpp

-      num_components =
-          static_cast<int>(n) -
-          Kokkos::create_mirror_view_and_copy(Kokkos::HostSpace{}, num_edges)();


Was this a bug?

Not really in the sense you get the right answer. The change here are about using the execution space provided to transfer to the host instead of the default one.