Provide Kokkos::realloc overload taking Kokkos::view_alloc() return type #5035

masterleinad · 2022-05-18T21:36:14Z

Supersedes #5027. This would allow passing a label or an execution space or other (possibly multiple) View constructor properties to Kokkos::realloc.
We decided in #5027 to not allow passing a label at all which is implemented here.

dalg24

Do we have somewhere an up-to-date overload set for resize, realloc, and create_mirror[_view]?

core/src/Kokkos_CopyViews.hpp

masterleinad · 2022-05-19T12:53:11Z

Do we have somewhere an up-to-date overload set for resize, realloc, and create_mirror[_view]?

The Wiki should be up-to-date, see https://github.com/kokkos/kokkos/wiki/Kokkos::resize, https://github.com/kokkos/kokkos/wiki/Kokkos::realloc and https://github.com/kokkos/kokkos/wiki/Kokkos::create_mirror.

resize and realloc at the moment can take a single View constructor property (which currently excludes labels and execution spaces) and create_mirror[_view] can take WithoutInitializing_t.

dalg24 · 2022-05-19T13:04:28Z

OK so none of them seem to be taking Kokkos::view_alloc() return type.

masterleinad · 2022-05-19T13:53:33Z

OK, so let's see what possible View constructor properties are and what we want:

	realloc	resize	create_mirror[_view][_and_copy]
labels	disallow	disallow	disallow
pointers	disallow	disallow	disallow
execution space	allow	allow	allow
memory space	disallow	disallow	allow
allow_padding	allow	allow	disallow
WithoutInitializing	allow	allow	allow

In my opinion, it makes sense to have an overload for the return type of view_alloc for all of them, at the very least, internally and we can then go from here. realloc can serve as a prototype for this.

masterleinad · 2022-05-22T15:07:57Z

I decided to implement the behavior for passing an execution space already. This needs #5040 to pass the tests and #4826 will remove some more fences that we currently need to allow in the test.

masterleinad · 2022-06-01T20:43:59Z

Retest this please.

masterleinad · 2022-06-29T19:56:11Z

Seems to work with Sacado.

masterleinad · 2022-07-07T17:47:43Z

cuda.debug_pin_um_to_host failing for CUDA-11.6-NVHPC looks unrelated/spurious:

7: [ RUN      ] cuda.debug_pin_um_to_host
7: Time CudaSpace: 0.006380 CudaUVMSpace_1: 0.015492 CudaUVMSpace_2: 0.016266 CudaPinnedHostSpace: 0.249478 CudaUVMSpace_Pinned: 0.016704
7: /var/jenkins/workspace/Kokkos/core/unit_test/cuda/TestCuda_DebugPinUVMSpace.cpp:128: Failure
7: Value of: passed
7:   Actual: false
7: Expected: true
7: [  FAILED  ] cuda.debug_pin_um_to_host (443 ms)

crtrott · 2022-07-08T15:15:33Z

Retest this please.

Old

core/unit_test/TestWithoutInitializing.hpp

core/src/Kokkos_CopyViews.hpp

PhilMiller · 2022-07-08T21:57:34Z

containers/src/Kokkos_DynRankView.hpp

+                "not include a pointer!");
+  static_assert(!alloc_prop_input::has_memory_space,
+                "The view constructor arguments passed to Kokkos::realloc must "
+                "not include a memory space instance!");

  const std::string label = v.label();

  v = drview_type();  // Deallocate first, if the only view to allocation


Not a flaw in this PR, but the global Kokkos::fence() implied by a deallocation does unfortunately undermine some of the goal of #4793.

Mentioning as much for the back-link over there as anything else.

The same applies for View as well

I'm very happy to discuss that again. I'm in favor in restricting to execution spaces that can actually access the memory space.

core/unit_test/tools/TestWithoutInitializing.cpp

PhilMiller · 2022-07-08T22:16:00Z

containers/src/Kokkos_DualView.hpp

@@ -925,6 +961,19 @@ class DualView : public ViewTraits<DataType, Arg1Type, Arg2Type, Arg3Type> {
      modified_flags(1) = modified_flags(0) = 0;


Not a behavior change in this PR that I can tell, but could @stanmoore1 or someone else familiar with DualView semantics check that in the case where size is the same and no reallocation happens, and we do initialize only on the device instance, that the resulting modified_flags values are sensible? It seems like a subsequent synchronization call should recognize that the re-initialized data on the device side is fresher than the stale data on the host side.

PhilMiller · 2022-07-08T22:18:47Z

containers/unit_tests/TestWithoutInitializing.hpp

+        outer_view2 = inner_view;
+      },
+      [&](BeginFenceEvent event) {
+        if (event.descriptor().find("HostSpace fence") != std::string::npos)


Can DynRankView exhibit the same "Debug Only Check for Execution Error" that the tests for DualView and ScatterView exclude?

The test for View has that exclusion, so I think we need it here, unless it's superfluous there.

Yes, this is only necessary for View since it includes an additional case.

core/src/Kokkos_CopyViews.hpp

Co-authored-by: Phil Miller <unmobile+gh@gmail.com>

masterleinad · 2022-07-10T13:36:33Z

Retest this please.

dalg24 · 2022-07-11T11:45:03Z

Failure (OpenMP timeout) is unrelated.

dalg24 · 2022-07-12T12:39:36Z

I messed up here

4: [ RUN      ] openmptarget.realloc_exec_space
4: Test failure: encountered unwanted events
4:   Found fence event!
4: BeginFence { "SharedAllocationRecord<Kokkos::Experimental::OpenMPTargetSpace, void>::SharedAllocationRecord(): fence after copying header from HostSpace", 0,6}
4: EndFence { 6}
4: BeginFence { "SharedAllocationRecord<Kokkos::Experimental::OpenMPTargetSpace, void>::SharedAllocationRecord(): fence after copying header from HostSpace", 67108864,7}
4: EndFence { 7}
4: BeginFence { "SharedAllocationRecord<Kokkos::Experimental::OpenMPTargetSpace, void>::SharedAllocationRecord(): fence after copying header from HostSpace", 0,8}
4: EndFence { 8}
4: BeginFence { "SharedAllocationRecord<Kokkos::Experimental::OpenMPTargetSpace, void>::SharedAllocationRecord(): fence after copying header from HostSpace", 67108864,9}
4: EndFence { 9}
4: /var/jenkins/workspace/Kokkos/core/unit_test/TestWithoutInitializing.hpp:132: Failure
4: Value of: success
4:   Actual: false
4: Expected: true
4: [  FAILED  ] openmptarget.realloc_exec_space (0 ms)

CI has been failing ever since the merge

masterleinad mentioned this pull request May 18, 2022

Allow specifying label in realloc #5027

Closed

dalg24 previously requested changes May 19, 2022

View reviewed changes

core/src/Kokkos_CopyViews.hpp Outdated Show resolved Hide resolved

core/src/Kokkos_CopyViews.hpp Show resolved Hide resolved

core/src/Kokkos_CopyViews.hpp Show resolved Hide resolved

masterleinad requested a review from dalg24 May 19, 2022 12:57

masterleinad mentioned this pull request May 20, 2022

Fix calling ZeroMemset in deep_copy #5040

Merged

masterleinad force-pushed the disallow_realloc_label branch from a2fcb14 to 5442b40 Compare May 31, 2022 22:02

cz4rs self-requested a review June 1, 2022 14:24

masterleinad added this to the Tentative 3.7 Release milestone Jun 6, 2022

masterleinad mentioned this pull request Jun 7, 2022

Allow ViewCtorProp in create_mirror[_view] #5095

Merged

masterleinad mentioned this pull request Jun 17, 2022

Provide ViewCtorProp overload for create_mirror_view_and_copy #5125

Merged

crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Jun 22, 2022

masterleinad mentioned this pull request Jun 29, 2022

Allow Kokkos::resize to take an execution space #4844

Merged

crtrott approved these changes Jun 30, 2022

View reviewed changes

masterleinad mentioned this pull request Jul 5, 2022

Document new realloc overloads taking Kokkos::view_alloc() return type kokkos/kokkos-core-wiki#103

Merged

masterleinad force-pushed the disallow_realloc_label branch 2 times, most recently from dc8c513 to 2b79fa2 Compare July 7, 2022 15:10

PhilMiller reviewed Jul 8, 2022

View reviewed changes

core/unit_test/TestWithoutInitializing.hpp Show resolved Hide resolved

PhilMiller reviewed Jul 8, 2022

View reviewed changes

core/unit_test/TestWithoutInitializing.hpp Outdated Show resolved Hide resolved

PhilMiller reviewed Jul 8, 2022

View reviewed changes

core/src/Kokkos_CopyViews.hpp Outdated Show resolved Hide resolved

PhilMiller reviewed Jul 8, 2022

View reviewed changes

masterleinad added 2 commits July 8, 2022 18:00

Provide Kokkos::realloc overload taking Kokkos::view_alloc() return type

baf7ea0

Fix up ScatterView

9501822

masterleinad added 11 commits July 8, 2022 18:00

Fix deep_copy with execution space from scalar to strided View

5445a28

Fix Threads backend

4050740

Fix GPU backends

9816df6

Also disallow memory spaces

983a568

Also disallow padding

f7eaa44

Use the correct label

056ee4c

Adapt for HostSpace fence

5e87ab1

Allow allowing padding

4324dfb

Use GTEST_SKIP; disable test for Threads

37e456d

Remove fences after allocate takes an execution space

0464d03

Use int32_t

54ed3e3

masterleinad force-pushed the disallow_realloc_label branch from 2b79fa2 to 54ed3e3 Compare July 8, 2022 22:09

PhilMiller reviewed Jul 8, 2022

View reviewed changes

core/unit_test/tools/TestWithoutInitializing.cpp Show resolved Hide resolved

PhilMiller reviewed Jul 8, 2022

View reviewed changes

Use GTEST_SKIP

0e47a9f

PhilMiller reviewed Jul 8, 2022

View reviewed changes

core/src/Kokkos_CopyViews.hpp Outdated Show resolved Hide resolved

masterleinad and others added 2 commits July 8, 2022 18:26

Update core/src/Kokkos_CopyViews.hpp

38eaca3

Co-authored-by: Phil Miller <unmobile+gh@gmail.com>

Remove a couple more fence exclusions

c3aea33

PhilMiller approved these changes Jul 8, 2022

View reviewed changes

nliber approved these changes Jul 8, 2022

View reviewed changes

masterleinad added 2 commits July 8, 2022 23:26

Reinstantiate fences

b9859b1

Fix warnings

8128972

masterleinad force-pushed the disallow_realloc_label branch from 7abebaf to 8128972 Compare July 10, 2022 00:05

dalg24 merged commit d0b31c6 into kokkos:develop Jul 11, 2022

masterleinad mentioned this pull request Jul 12, 2022

Disable openmptarget.realloc_exec_space #5204

Merged

masterleinad deleted the disallow_realloc_label branch July 12, 2022 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide Kokkos::realloc overload taking Kokkos::view_alloc() return type #5035

Provide Kokkos::realloc overload taking Kokkos::view_alloc() return type #5035

masterleinad commented May 18, 2022 •

edited

dalg24 left a comment

masterleinad commented May 19, 2022

dalg24 commented May 19, 2022

masterleinad commented May 19, 2022 •

edited

masterleinad commented May 22, 2022

masterleinad commented Jun 1, 2022

masterleinad commented Jun 29, 2022

masterleinad commented Jul 7, 2022

crtrott commented Jul 8, 2022

PhilMiller Jul 8, 2022

PhilMiller Jul 8, 2022

masterleinad Jul 8, 2022

PhilMiller Jul 8, 2022

PhilMiller Jul 8, 2022

PhilMiller Jul 8, 2022

masterleinad Jul 8, 2022

masterleinad commented Jul 10, 2022

dalg24 commented Jul 11, 2022

dalg24 commented Jul 12, 2022

		@@ -925,6 +961,19 @@ class DualView : public ViewTraits<DataType, Arg1Type, Arg2Type, Arg3Type> {
		modified_flags(1) = modified_flags(0) = 0;

Provide Kokkos::realloc overload taking Kokkos::view_alloc() return type #5035

Provide Kokkos::realloc overload taking Kokkos::view_alloc() return type #5035

Conversation

masterleinad commented May 18, 2022 • edited

dalg24 left a comment

Choose a reason for hiding this comment

masterleinad commented May 19, 2022

dalg24 commented May 19, 2022

masterleinad commented May 19, 2022 • edited

masterleinad commented May 22, 2022

masterleinad commented Jun 1, 2022

masterleinad commented Jun 29, 2022

masterleinad commented Jul 7, 2022

crtrott commented Jul 8, 2022

PhilMiller Jul 8, 2022

Choose a reason for hiding this comment

PhilMiller Jul 8, 2022

Choose a reason for hiding this comment

masterleinad Jul 8, 2022

Choose a reason for hiding this comment

PhilMiller Jul 8, 2022

Choose a reason for hiding this comment

PhilMiller Jul 8, 2022

Choose a reason for hiding this comment

PhilMiller Jul 8, 2022

Choose a reason for hiding this comment

masterleinad Jul 8, 2022

Choose a reason for hiding this comment

masterleinad commented Jul 10, 2022

dalg24 commented Jul 11, 2022

dalg24 commented Jul 12, 2022

masterleinad commented May 18, 2022 •

edited

masterleinad commented May 19, 2022 •

edited