Fix combined reductions with Kokkos::View #4896

masterleinad · 2022-03-23T18:43:07Z

Extracted from #4889. We should preserve the memory space in combined reductions if a View is given.

dalg24 · 2022-03-23T19:20:16Z

core/unit_test/TestReduce.hpp

@@ -635,7 +635,7 @@ TEST(TEST_CATEGORY, int_combined_reduce_mixed) {

  uint64_t nsum = (nw / 2) * (nw + 1);

-  auto result1_v = Kokkos::View<int64_t, Kokkos::HostSpace>{"result1_v"};
+  auto result1_v = Kokkos::View<int64_t>{"result1_v"};


Why did you change the test. This looks intentional.

It is intentional.

I changed it to demonstrate the issue that I encountered in #4889 that was failing with an incompatible View assignment error (without changing anything in this test). Is there are reason to only allow HostSpace here?

Oh I see. Can we change the test to take four reduction args, i.e. result1_v with defaultspace, result2_v with HostSpace, result3 as a scalar, and a fourth using the reducers (which is wrapping a host view)?

crtrott · 2022-03-23T19:20:54Z

core/unit_test/TestReduce.hpp

@@ -635,7 +635,7 @@ TEST(TEST_CATEGORY, int_combined_reduce_mixed) {

  uint64_t nsum = (nw / 2) * (nw + 1);

-  auto result1_v = Kokkos::View<int64_t, Kokkos::HostSpace>{"result1_v"};
+  auto result1_v = Kokkos::View<int64_t>{"result1_v"};


It is intentional.

crtrott · 2022-03-23T19:38:06Z

core/unit_test/TestReduce.hpp

@@ -635,7 +635,7 @@ TEST(TEST_CATEGORY, int_combined_reduce_mixed) {

  uint64_t nsum = (nw / 2) * (nw + 1);

-  auto result1_v = Kokkos::View<int64_t, Kokkos::HostSpace>{"result1_v"};
+  auto result1_v = Kokkos::View<int64_t>{"result1_v"};


Oh I see. Can we change the test to take four reduction args, i.e. result1_v with defaultspace, result2_v with HostSpace, result3 as a scalar, and a fourth using the reducers (which is wrapping a host view)?

masterleinad · 2022-03-24T14:34:18Z

This will require some more changes so I'll convert it to a draft again.

masterleinad · 2022-03-24T20:52:49Z

It turned out that making this work for memory spaces that are not host-accessible is not that trivial. The combined reducer implementation basically forms a reducer that lives in host memory space. To construct the underlying values, the implementation so far tried to access the VIew via the call operator (in host execution space) if a View was given as reducer which obviously doesn't work if the View is allocated in non-host-accessible memory space.
Since the initial value isn't used anyway, I decided to default-construct one of the desired type instead of copying the one we have stored in the View. Since the value type in a View has to be default-constructible anyway (implicitly in the current implementation), that should work.
When the value stored in the combined reducer is copied back, we run into the same memory access problems and we just perform a Kokkos::deep_copy now if the memory space is not accessible from the host.

In the end, using View with non-host-accessible memory space in combined reductions is more expensive than a host-accessible View + deep_copy. Hence, we could also decide to restrict combined reductions to host-accessible memory spaces. Note that the original fix changing the memory space of the implicitly constructed view type is still required for #4889.

masterleinad · 2022-03-25T15:50:26Z

Retest this please.

masterleinad · 2022-03-25T17:22:20Z

Only building the OpenMPTarget-Clang image failed.

masterleinad · 2022-04-06T20:59:38Z

Retest this please.

core/src/impl/Kokkos_Combined_Reducer.hpp

…uctions

core/src/impl/Kokkos_Combined_Reducer.hpp

dalg24

Looks good now. Can you think of a better name for write_one_value_back?

Did we ever discuss the semantics when mixing value and view? Obviously now, if one of the argument is a view, then it is asynchronous.

dalg24 · 2022-06-02T22:33:04Z

core/src/impl/Kokkos_Combined_Reducer.hpp

@@ -266,13 +265,26 @@ struct CombinedReducerImpl<std::integer_sequence<size_t, Idxs...>, Space,
    return m_value_view;
  }

-  KOKKOS_FUNCTION
+  template <class ExecutionSpace, int Idx, class View>
+  constexpr static void write_one_value_back(


constexpr is superfluous. There is no way it works in a constexpr context, neither the deep copy nor the assign to rank-0 view

I only added it because the calling one was constexpr. Do you want me to drop it?

I'd say drop it.

And if the caller (I assume that is write_value_back_to_original_references(...)) is constexpr, that can't be correct now either, and should also have constexpr dropped.

masterleinad · 2022-06-03T21:12:53Z

Looks good now. Can you think of a better name for write_one_value_back?

I thought it's an appropriate name given that it's called by write_value_back_to_original_references().

Did we ever discuss the semantics when mixing value and view? Obviously now, if one of the argument is a view, then it is asynchronous.

I would think that we as a whole didn't spend much time talking about the semantics of combined reductions at all. I would rather say that we need to fence whenever one of the arguments is a value.

dalg24

Sharing pending change I had. Haven't reviewed the whole thing.

core/src/impl/Kokkos_Combined_Reducer.hpp

nliber · 2022-06-15T17:49:10Z

core/src/impl/Kokkos_Combined_Reducer.hpp

@@ -266,13 +265,26 @@ struct CombinedReducerImpl<std::integer_sequence<size_t, Idxs...>, Space,
    return m_value_view;
  }

-  KOKKOS_FUNCTION
+  template <class ExecutionSpace, int Idx, class View>
+  constexpr static void write_one_value_back(


I'd say drop it.

And if the caller (I assume that is write_value_back_to_original_references(...)) is constexpr, that can't be correct now either, and should also have constexpr dropped.

masterleinad · 2022-06-15T21:38:08Z

@nliber I dropped constexpr for the two functions.

masterleinad · 2022-06-17T20:57:24Z

@crtrott @nliber ping I addressed your comments.

Done

Fix combined reductions with Kokkos::View

00ecf8f

nliber approved these changes Mar 23, 2022

View reviewed changes

PhilMiller approved these changes Mar 23, 2022

View reviewed changes

dalg24 reviewed Mar 23, 2022

View reviewed changes

crtrott requested changes Mar 23, 2022

View reviewed changes

masterleinad marked this pull request as draft March 24, 2022 14:34

masterleinad force-pushed the fix_combined_reductions branch 2 times, most recently from 9ddebb1 to d7528f6 Compare March 24, 2022 20:38

Fix memory access problems

28cb6bd

masterleinad force-pushed the fix_combined_reductions branch from d7528f6 to 28cb6bd Compare March 24, 2022 20:41

masterleinad marked this pull request as ready for review March 24, 2022 20:41

masterleinad added 2 commits April 1, 2022 17:00

Instead of modifying copy existing test using device memory space

0588ec5

Fix typo

5826891

masterleinad force-pushed the fix_combined_reductions branch from 51ec1f6 to e3fdc3c Compare April 5, 2022 15:04

dalg24 reviewed Jun 2, 2022

View reviewed changes

core/src/impl/Kokkos_Combined_Reducer.hpp Outdated Show resolved Hide resolved

core/src/impl/Kokkos_Combined_Reducer.hpp Outdated Show resolved Hide resolved

core/src/impl/Kokkos_Combined_Reducer.hpp Outdated Show resolved Hide resolved

masterleinad added 2 commits June 2, 2022 17:51

Merge remote-tracking branch 'upstream/develop' into fix_combined_red…

71f7a59

…uctions

Use three-argument deep_copy

f6a49d5

masterleinad force-pushed the fix_combined_reductions branch from e3fdc3c to f6a49d5 Compare June 2, 2022 21:53

dalg24 reviewed Jun 2, 2022

View reviewed changes

core/src/impl/Kokkos_Combined_Reducer.hpp Outdated Show resolved Hide resolved

dalg24 reviewed Jun 2, 2022

View reviewed changes

core/src/impl/Kokkos_Combined_Reducer.hpp Outdated Show resolved Hide resolved

typename ExecutionSpace -> class ExecutionSpace

26351b9

masterleinad requested a review from crtrott June 2, 2022 22:13

dalg24 reviewed Jun 2, 2022

View reviewed changes

Also default construct a value for the reducer case

d80d065

masterleinad force-pushed the fix_combined_reductions branch from 124dbac to d80d065 Compare June 6, 2022 16:18

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

dalg24 requested review from PhilMiller and nliber June 8, 2022 02:35

PhilMiller approved these changes Jun 8, 2022

View reviewed changes

dalg24 reviewed Jun 8, 2022

View reviewed changes

core/src/impl/Kokkos_Combined_Reducer.hpp Outdated Show resolved Hide resolved

Combine function specializations

78206f1

nliber previously requested changes Jun 15, 2022

View reviewed changes

Drop constexpr for write_value_back_to_original_references

8b87cf8

masterleinad requested a review from nliber June 15, 2022 21:37

dalg24 approved these changes Jun 20, 2022

View reviewed changes

crtrott approved these changes Jun 20, 2022

View reviewed changes

crtrott merged commit d3c1948 into kokkos:develop Jun 20, 2022

masterleinad mentioned this pull request Sep 7, 2022

Add CI run with HBWSpace #4889

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix combined reductions with Kokkos::View #4896

Fix combined reductions with Kokkos::View #4896

masterleinad commented Mar 23, 2022

dalg24 Mar 23, 2022

crtrott Mar 23, 2022

masterleinad Mar 23, 2022 •

edited

crtrott Mar 23, 2022

crtrott Mar 23, 2022

crtrott Mar 23, 2022

masterleinad commented Mar 24, 2022

masterleinad commented Mar 24, 2022

masterleinad commented Mar 25, 2022

masterleinad commented Mar 25, 2022

masterleinad commented Apr 6, 2022

dalg24 left a comment

dalg24 Jun 2, 2022

masterleinad Jun 3, 2022

nliber Jun 15, 2022

masterleinad commented Jun 3, 2022

dalg24 left a comment

nliber Jun 15, 2022

masterleinad commented Jun 15, 2022

masterleinad commented Jun 17, 2022

Fix combined reductions with Kokkos::View #4896

Fix combined reductions with Kokkos::View #4896

Conversation

masterleinad commented Mar 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masterleinad Mar 23, 2022 • edited

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

masterleinad commented Mar 24, 2022

masterleinad commented Mar 24, 2022

masterleinad commented Mar 25, 2022

masterleinad commented Mar 25, 2022

masterleinad commented Apr 6, 2022

dalg24 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

masterleinad commented Jun 3, 2022

dalg24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masterleinad commented Jun 15, 2022

masterleinad commented Jun 17, 2022

masterleinad Mar 23, 2022 •

edited