OpenMPTarget: Adding Implementation for nested reducers #3845

rgayatri23 · 2021-03-11T20:32:02Z

The PR contains the following updates in the OpenMPTarget backend:

nested array reduction
nested kokkos-reducers
nested reducers with join

Major contributor to the PR - Patrick Steinbrecher (@psteinbrecher)

…ion, kokkos-reducers.

masterleinad · 2021-03-11T21:39:17Z

Major contributor to the PR - Patrick Steinbrecher (@psteinbrecher)

Then he should also co-author the commits here (https://docs.github.com/en/github/committing-changes-to-your-project/creating-a-commit-with-multiple-authors). 🙂

masterleinad · 2021-03-11T21:41:01Z

You should enable the corresponding tests in this pull request.

rgayatri23 · 2021-03-12T00:39:21Z

You should enable the corresponding tests in this pull request.

Yes working on the unit tests right now. Will update them soon.

masterleinad · 2021-03-12T18:29:16Z

The indentation is off. 😉

…the corresponding unit tests.

crtrott · 2021-03-15T15:51:16Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp

+#pragma omp barrier
+
+  if constexpr (std::is_arithmetic<ValueType>::value) {
+#pragma omp for reduction(+ : tmp_scratch[:1])


this is wrong: it does + for the inter thread reduction, and then join within a thread.

…kkos-reducers.

…e. Wrapped the commented code in ifdefs.

dhollman

There is way too much new code duplication in this pull request for me to be comfortable saying I could maintain it. Please consolidate.

dhollman · 2021-03-24T18:41:45Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Parallel.hpp

+template <class FunctorType, class ReducerType, class PointerType,
+          class ValueType, class... PolicyArgs>
+struct ParallelReduceSpecialize<FunctorType, TeamPolicyInternal<PolicyArgs...>,
+                                ReducerType, PointerType, ValueType, 0, 1> {


/* functor has join = */ 0, /* use reducer type = */ 1

Also, why are these ints instead of bools?

You should still address this.

dhollman · 2021-03-24T18:48:14Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Parallel.hpp

+
+#pragma omp declare reduction(                                         \
+    custom:ValueType                                                   \
+    : OpenMPTargetReducerWrapper <ReducerType>::join(omp_out, omp_in)) \


practically everything except for this pragma here is copied from the 0, 0 case above, but the only way to figure that out is to run diff on the code segments. This is way too much code duplication for me to feel comfortable approving this. It needs to be restructured so that the repeated lines aren't in two places; otherwise we can't maintain this.

dhollman · 2021-03-24T18:49:16Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Parallel.hpp

+  inline static
+      typename std::enable_if<!std::is_same<TagType, void>::value>::type
+      execute_impl(const FunctorType& f, const PolicyType& p,
+                   PointerType result_ptr) {


This differs by one line from the 50+ lines of code in the non-tag version. Please refactor this to remove the copy-pasted code.

This issue is addressed . The check for tag is pushed inside the function and the duplicated function is deleted.

dhollman · 2021-03-24T18:51:26Z

core/unit_test/TestTeam.hpp

@@ -1286,8 +1286,16 @@ struct TestTeamBroadcast<
    using policy_type_f =
        Kokkos::TeamPolicy<ScheduleType, ExecSpace, BroadcastTag>;

+    // FIXME_OPENMPTARGET temporary restriction for team size to be at least 32
+#ifdef KOKKOS_ENABLE_OPENMPTARGET


Since execution spaces are unconditionally declared now, we shouldn't need this ifdef any more (right)?

No, we haven't merged #3838.

rgayatri23 · 2021-03-24T19:06:16Z

Major contributor to the PR - Patrick Steinbrecher (@psteinbrecher)

Then he should also co-author the commits here (https://docs.github.com/en/github/committing-changes-to-your-project/creating-a-commit-with-multiple-authors). 🙂

Unfortunately he cannot be associated with any PRs due to conflicts in his work policy.

rgayatri23 · 2021-03-24T19:14:12Z

There is way too much new code duplication in this pull request for me to be comfortable saying I could maintain it. Please consolidate.

I was thinking of consolidating the duplicated code in a separate PR once this gets merged. It has been an issue since before this PR. But if there is a strong opinion on doing it in this PR itself, I have no issues.

dhollman · 2021-03-24T19:30:46Z

I was thinking of consolidating the duplicated code in a separate PR once this gets merged. It has been an issue since before this PR. But if there is a strong opinion on doing it in this PR itself, I have no issues.

If other reviewers think that's fine, then I'm okay with it. I just think it's easier to avoid producing duplicate code in the first place, though. At the very least, mark the duplicated lines with comments like "begin copy-paste X" and "end copy-paste from X" so that we can remove it later.

…shed the check inside.

dalg24 · 2021-03-26T03:02:36Z

core/unit_test/TestTeamBasic.hpp

@@ -66,7 +66,8 @@ TEST(TEST_CATEGORY, team_for) {
 }

 // FIXME_OPENMPTARGET wrong results
-#ifndef KOKKOS_ENABLE_OPENMPTARGET
+// FIXME_SYCL team reduction and broadcast not yet implemented
+#if !defined(KOKKOS_ENABLE_SYCL) && !defined(KOKKOS_ENABLE_OPENMPTARGET)


That looks like an involuntary change. @masterleinad

Yes, this works for SYCL.

I will remove the ifdef around SYCL

dalg24 · 2021-03-26T03:10:47Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp

 }
+#endif  // KOKKOS_IMPL_HIERARCHICAL_REDUCERS_WORKAROUND


I would undef KOKKOS_IMPL_HIERARCHICAL_REDUCERS_WORKAROUND right there

dalg24 · 2021-03-26T03:12:40Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp

-        value = *local_value;
-    #endif*/
+  KOKKOS_INLINE_FUNCTION void team_broadcast(ValueType& value,
+                                             const int& thread_id) const {


We need to quit passing arithmetic types by const reference.

masterleinad · 2021-03-26T15:09:28Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp

@@ -51,6 +51,11 @@
 #include <Kokkos_Atomic.hpp>
 #include "Kokkos_OpenMPTarget_Abort.hpp"

+// FIXME_OPENMPTARGET - Using this macro to implement a workaround for
+// hierarchical reducers. It avoids hitting the code path which we wanted to
+// write but dosen't work. undef'ed at the end.


Suggested change

// write but dosen't work. undef'ed at the end.

// write but doesn't work. undef'ed at the end.

masterleinad · 2021-03-26T15:22:21Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp

+    using type = typename if_c<sizeof(ValueType) < TEAM_REDUCE_SIZE, ValueType,
+                               void>::type;


Why don't you use std::conditional_t here?

masterleinad · 2021-03-26T15:23:33Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp

+    using type = typename if_c<sizeof(ValueType) < TEAM_REDUCE_SIZE, ValueType,
+                               void>::type;
+    type* team_scratch =
+        (type*)((char*)m_glb_scratch + TEAM_REDUCE_SIZE * omp_get_team_num());


C-style casts for pointers 🙁

masterleinad · 2021-03-26T15:32:06Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp

+    typename std::enable_if<!Kokkos::is_reducer_type<ValueType>::value>::type
+    parallel_reduce(


Can't you implement this one in terms of the reducer implementation using Kokkos::Sum?

masterleinad · 2021-03-26T15:32:29Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp

+
+#if !defined(KOKKOS_IMPL_HIERARCHICAL_REDUCERS_WORKAROUND)
+// For some reason the actual version we wanted to write doesn't work
+// and crashes. We should trye this with every new compiler


Suggested change

// and crashes. We should trye this with every new compiler

// and crashes. We should try this with every new compiler

masterleinad · 2021-03-26T15:37:35Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp

+  static_assert(sizeof(ValueType) <=
+                Impl::OpenMPTargetExecTeamMember::TEAM_REDUCE_SIZE);


Is this a FIXME?

Can't this be implemented in terms of the reducer variant using Kokkos::Sum?

Is this a FIXME?

No this is not a FIXME, essentially we are restricting the nested array-reduction size to be <= 32 elements per team.
The reduction uses a global scratch, so 32 elements with 16 bytes per element is essentially 512 bytes per team. Anything more than this might be detrimental for the performance.

Can't this be implemented in terms of the reducer variant using Kokkos::Sum?

We were trying to rely on OpenMP constructs as much as possible.

Maybe I am misunderstanding but does that mean that you can't do reductions for large types (and that is never intended to work)?

I want to say its better to not perform reductions larger than 32 elements as long as we have the current implementation strategy where we allocate 16 bytes per element in the scratch memory. But maybe @crtrott can answer this better.

May question solely is if we should put a FIXME there or not. 🙂

Yes you are right. We should put a FIXME there. Will do it.

Actually I forgot that we have already added a FIXME_OPENMPTARGET for this issue in one of the variants of the parallel_reduce functions. I will repeat that everywhere.

This static_assert seems to be duplicated now looking at lines 1573-1577.

… with c++ style. Added FIXME messages to the restriction of 32 elements per team in the array reduction of parallel_reduce.

…est.

masterleinad · 2021-03-30T16:59:51Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp

+  static_assert(sizeof(ValueType) <=
+                Impl::OpenMPTargetExecTeamMember::TEAM_REDUCE_SIZE);


This one seems duplicated looking at line 1614-1618.

dalg24

You forgot to undef KOKKOS_IMPL_HIERARCHICAL_REDUCERS_WORKAROUND

dalg24 · 2021-03-29T18:41:37Z

core/unit_test/TestTeamBasic.hpp

-#ifdef KOKKOS_ENABLE_OPENMPTARGET
-  if constexpr (!std::is_same<TEST_EXECSPACE,
-                              Kokkos::Experimental::OpenMPTarget>::value)
-#endif
  {


Suggested change

{

Same comment below

dalg24 · 2021-03-29T18:44:13Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp

 }
+#endif  // KOKKOS_IMPL_HIERARCHICAL_REDUCERS_WORKAROUND


dalg24 · 2021-03-30T16:04:58Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp

+  static_assert(sizeof(ValueType) <=
+                Impl::OpenMPTargetExecTeamMember::TEAM_REDUCE_SIZE);
+
+  ValueType* TeamThread_scratch =


Style: prefer team_thread_scratch

I wanted it to match TeamThreadRangeBoundariesStruct in the function definition. But I can change it if its about being consistent in the way we name variables.

Not blocking

dalg24 · 2021-03-30T16:50:01Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp

-    const Lambda& lambda, ValueType& result) {
-  ValueType* tmp_scratch =
+KOKKOS_INLINE_FUNCTION
+    typename std::enable_if<!Kokkos::is_reducer_type<ValueType>::value>::type


Suggested change

typename std::enable_if<!Kokkos::is_reducer_type<ValueType>::value>::type

std::enable_if_t<!Kokkos::is_reducer_type<ValueType>::value>

dalg24 · 2021-03-30T16:54:27Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp

+// This is the variant we actually wanted to write
+template <typename iType, class Lambda, typename ReducerType>
+KOKKOS_INLINE_FUNCTION
+    typename std::enable_if<Kokkos::is_reducer_type<ReducerType>::value>::type


Suggested change

typename std::enable_if<Kokkos::is_reducer_type<ReducerType>::value>::type

std::enable_if_t<Kokkos::is_reducer_type<ReducerType>::value>

masterleinad

Looks OK to me.

Changes requested were partially addressed and will be fully addressed in follow-up PR

OpenMPTarget: Adding Implementation for nested reducers, array reduct…

92875b3

…ion, kokkos-reducers.

rgayatri23 added the [WIP] label Mar 11, 2021

rgayatri23 requested a review from crtrott March 11, 2021 20:32

rgayatri23 removed the [WIP] label Mar 12, 2021

OpenMPTarget: Bug fixes to nested reduction implementation. Enabling …

2b4b3b1

…the corresponding unit tests.

crtrott reviewed Mar 15, 2021

View reviewed changes

rgayatri23 added the [WIP] label Mar 15, 2021

rgayatri added 2 commits March 15, 2021 11:36

OpenMPTarget: Fixed the reduction error in parallel_reduce.

1b07db1

OpenMPTarget: Adding ParallelReduceSpecialize for team-policy over ko…

7320616

…kkos-reducers.

crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Mar 15, 2021

dhollman self-requested a review March 17, 2021 20:41

crtrott and others added 6 commits March 18, 2021 21:07

OpenMPTarget: Add missing reduction clause to TeamPolicy reduce

e923740

OpenMPTarget work around compiler issue for nested reducers

02dd97f

OpenMPTarget: More TeamPolicy reduction fixes

b73dd72

OpenMPTarget: implement team_broadcast

9a753d0

OpenMPTarget: enable team_broadcast tests

cb238bc

OpenMPTarget: Replaced scratch pointer names in nested parallel_reduc…

e7f909b

…e. Wrapped the commented code in ifdefs.

rgayatri23 removed the [WIP] label Mar 24, 2021

dhollman previously requested changes Mar 24, 2021

View reviewed changes

Merge branch 'develop' into OpenMPTarget_NestedReducers

916d6ed

rgayatri and others added 3 commits March 24, 2021 12:43

OpenMPTarget: Fixing merge conflicts.

b0a8a63

OpenMPTarget: Removed duplicate functions for TagTypes and instead pu…

acb436d

…shed the check inside.

Merge branch 'develop' into OpenMPTarget_NestedReducers

1dd9296

dalg24 reviewed Mar 26, 2021

View reviewed changes

masterleinad reviewed Mar 26, 2021

View reviewed changes

rgayatri added 2 commits March 26, 2021 13:21

OpenMPTarget: Replaced if_c with std::conditional and c-style casting…

9c57dfd

… with c++ style. Added FIXME messages to the restriction of 32 elements per team in the array reduction of parallel_reduce.

OpenMPTarget: Removed the ifndef for SYCL in the TestTeamBasic unit t…

068431b

…est.

masterleinad reviewed Mar 30, 2021

View reviewed changes

dalg24 reviewed Mar 30, 2021

View reviewed changes

OpenMPTarget: Removed duplicated checks in parallel_reduce.

71d64d2

masterleinad approved these changes Mar 30, 2021

View reviewed changes

OpenMPTarget: Replaced int with bool in ParallelReduce speacialization.

0639244

dalg24 approved these changes Mar 31, 2021

View reviewed changes

crtrott approved these changes Mar 31, 2021

View reviewed changes

dalg24 merged commit 8b7a01f into kokkos:develop Apr 7, 2021

rgayatri23 deleted the OpenMPTarget_NestedReducers branch March 24, 2023 22:03

	// write but dosen't work. undef'ed at the end.
	// write but doesn't work. undef'ed at the end.

		using type = typename if_c<sizeof(ValueType) < TEAM_REDUCE_SIZE, ValueType,
		void>::type;

		typename std::enable_if<!Kokkos::is_reducer_type<ValueType>::value>::type
		parallel_reduce(

	// and crashes. We should trye this with every new compiler
	// and crashes. We should try this with every new compiler

		static_assert(sizeof(ValueType) <=
		Impl::OpenMPTargetExecTeamMember::TEAM_REDUCE_SIZE);

	typename std::enable_if<!Kokkos::is_reducer_type<ValueType>::value>::type
	std::enable_if_t<!Kokkos::is_reducer_type<ValueType>::value>

	typename std::enable_if<Kokkos::is_reducer_type<ReducerType>::value>::type
	std::enable_if_t<Kokkos::is_reducer_type<ReducerType>::value>

		}
		#endif // KOKKOS_IMPL_HIERARCHICAL_REDUCERS_WORKAROUND

		}
		#endif // KOKKOS_IMPL_HIERARCHICAL_REDUCERS_WORKAROUND

OpenMPTarget: Adding Implementation for nested reducers #3845

OpenMPTarget: Adding Implementation for nested reducers #3845

Conversation

rgayatri23 commented Mar 11, 2021

masterleinad commented Mar 11, 2021

masterleinad commented Mar 11, 2021

rgayatri23 commented Mar 12, 2021

masterleinad commented Mar 12, 2021

Choose a reason for hiding this comment

dhollman 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

rgayatri23 commented Mar 24, 2021

rgayatri23 commented Mar 24, 2021

dhollman commented Mar 24, 2021

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

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

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

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 left a comment

Choose a reason for hiding this comment