Fix PR30890: Reduction across teams hangs

[Hahnfeld]

Summary

__kmpc_reduce_nowait() correctly swapped the teams for reductions
in a teams construct. Apply the same logic to __kmpc_reduce() and
__kmpc_reduce_end().

[AndreyChurbanov]

LGTM

[Hahnfeld]

openmp/trunk/runtime/test/misc_bugs/teams-reduction.c

33–34

The compiler only aligns this to 4 byte, the size of one int32_t. This results in a SIGBUS error on Power because the runtime stores a pointer so the memory location needs to be aligned to 8 bytes. I've solved this in rL319811 with

typedef union {
  // The global will be used as pointer, so we need to make sure that the
  // compiler correctly aligns the global...
  void *ptr;
  int32_t data[8];
} kmp_critical_name;

If somebody has a less ugly idea, I'm happy to listen but I didn't want to use compiler dependent alignment directives...

openmp/trunk/runtime/src/kmp_csupport.cpp

#else // KMP_USE_DYNAMIC_LOCK
   if (__kmp_env_consistency_check)
     __kmp_pop_sync(global_tid, ct_critical, loc);
 
   __kmp_release_user_lock_with_checks(lck, global_tid);
 
 #endif // KMP_USE_DYNAMIC_LOCK
 } // __kmp_end_critical_section_reduce_block
 
+#if OMP_40_ENABLED
+static __forceinline int
+__kmp_swap_teams_for_teams_reduction(kmp_info_t *th, kmp_team_t **team_p,
+                                     int *task_state) {
+  kmp_team_t *team;
+
+  // Check if we are inside the teams construct?
+  if (th->th.th_teams_microtask) {
+    *team_p = team = th->th.th_team;
+    if (team->t.t_level == th->th.th_teams_level) {
+      // This is reduction at teams construct.
+      KMP_DEBUG_ASSERT(!th->th.th_info.ds.ds_tid); // AC: check that tid == 0
+      // Let's swap teams temporarily for the reduction.
+      th->th.th_info.ds.ds_tid = team->t.t_master_tid;
+      th->th.th_team = team->t.t_parent;
+      th->th.th_team_nproc = th->th.th_team->t.t_nproc;
+      th->th.th_task_team = th->th.th_team->t.t_task_team[0];
+      *task_state = th->th.th_task_state;
+      th->th.th_task_state = 0;
+
+      return 1;
+    }
+  }
+  return 0;
+}
+
+static __forceinline void
+__kmp_restore_swapped_teams(kmp_info_t *th, kmp_team_t *team, int task_state) {
+  // Restore thread structure swapped in __kmp_swap_teams_for_teams_reduction.
+  th->th.th_info.ds.ds_tid = 0;
+  th->th.th_team = team;
+  th->th.th_team_nproc = team->t.t_nproc;
+  th->th.th_task_team = team->t.t_task_team[task_state];
+  th->th.th_task_state = task_state;
+}
+#endif
+
 /* 2.a.i. Reduce Block without a terminating barrier */
 /*!
 @ingroup SYNCHRONIZATION
 @param loc source location information
 @param global_tid global thread number
 @param num_vars number of items (variables) to be reduced
 @param reduce_size size of data in bytes to be reduced
 @param reduce_data pointer to data to be reduced
__kmpc_reduce_nowait(ident_t *loc, kmp_int32 global_tid, kmp_int32 num_vars,
                      size_t reduce_size, void *reduce_data,
                      void (*reduce_func)(void *lhs_data, void *rhs_data),
                      kmp_critical_name *lck) {
 
   KMP_COUNT_BLOCK(REDUCE_nowait);
   int retval = 0;
   PACKED_REDUCTION_METHOD_T packed_reduction_method;
 #if OMP_40_ENABLED
-  kmp_team_t *team;
   kmp_info_t *th;
+  kmp_team_t *team;
   int teams_swapped = 0, task_state;
 #endif
   KA_TRACE(10, ("__kmpc_reduce_nowait() enter: called T#%d\n", global_tid));
 
   // why do we need this initialization here at all?
   // Reduction clause can not be used as a stand-alone directive.
 
   // do not call __kmp_serial_initialize(), it will be called by
   // __kmp_parallel_initialize() if needed
   // possible detection of false-positive race by the threadchecker ???
   if (!TCR_4(__kmp_init_parallel))
     __kmp_parallel_initialize();
 
 // check correctness of reduce block nesting
 #if KMP_USE_DYNAMIC_LOCK
   if (__kmp_env_consistency_check)
     __kmp_push_sync(global_tid, ct_reduce, loc, NULL, 0);
 #else
   if (__kmp_env_consistency_check)
     __kmp_push_sync(global_tid, ct_reduce, loc, NULL);
 #endif
 
 #if OMP_40_ENABLED
   th = __kmp_thread_from_gtid(global_tid);
-  if (th->th.th_teams_microtask) { // AC: check if we are inside the teams
-    // construct?
-    team = th->th.th_team;
-    if (team->t.t_level == th->th.th_teams_level) {
-      // this is reduction at teams construct
-      KMP_DEBUG_ASSERT(!th->th.th_info.ds.ds_tid); // AC: check that tid == 0
-      // Let's swap teams temporarily for the reduction barrier
-      teams_swapped = 1;
-      th->th.th_info.ds.ds_tid = team->t.t_master_tid;
-      th->th.th_team = team->t.t_parent;
-      th->th.th_team_nproc = th->th.th_team->t.t_nproc;
-      th->th.th_task_team = th->th.th_team->t.t_task_team[0];
-      task_state = th->th.th_task_state;
-      th->th.th_task_state = 0;
-    }
-  }
+  teams_swapped = __kmp_swap_teams_for_teams_reduction(th, &team, &task_state);
 #endif // OMP_40_ENABLED
 
   // packed_reduction_method value will be reused by __kmp_end_reduce* function,
   // the value should be kept in a variable
   // the variable should be either a construct-specific or thread-specific
   // property, not a team specific property
   //     (a thread can reach the next reduce block on the next construct, reduce
   //     method may differ on the next construct)
#endif
 
   } else {
 
     // should never reach this block
     KMP_ASSERT(0); // "unexpected method"
   }
 #if OMP_40_ENABLED
   if (teams_swapped) {
-    // Restore thread structure
-    th->th.th_info.ds.ds_tid = 0;
-    th->th.th_team = team;
-    th->th.th_team_nproc = team->t.t_nproc;
-    th->th.th_task_team = team->t.t_task_team[task_state];
-    th->th.th_task_state = task_state;
+    __kmp_restore_swapped_teams(th, team, task_state);
   }
 #endif
   KA_TRACE(
       10,
       ("__kmpc_reduce_nowait() exit: called T#%d: method %08x, returns %08x\n",
        global_tid, packed_reduction_method, retval));
 
   return retval;

 */
 kmp_int32 __kmpc_reduce(ident_t *loc, kmp_int32 global_tid, kmp_int32 num_vars,
                         size_t reduce_size, void *reduce_data,
                         void (*reduce_func)(void *lhs_data, void *rhs_data),
                         kmp_critical_name *lck) {
   KMP_COUNT_BLOCK(REDUCE_wait);
   int retval = 0;
   PACKED_REDUCTION_METHOD_T packed_reduction_method;
+#if OMP_40_ENABLED
+  kmp_info_t *th;
+  kmp_team_t *team;
+  int teams_swapped = 0, task_state;
+#endif
 
   KA_TRACE(10, ("__kmpc_reduce() enter: called T#%d\n", global_tid));
 
   // why do we need this initialization here at all?
   // Reduction clause can not be a stand-alone directive.
 
   // do not call __kmp_serial_initialize(), it will be called by
   // __kmp_parallel_initialize() if needed
   // possible detection of false-positive race by the threadchecker ???
   if (!TCR_4(__kmp_init_parallel))
     __kmp_parallel_initialize();
 
 // check correctness of reduce block nesting
 #if KMP_USE_DYNAMIC_LOCK
   if (__kmp_env_consistency_check)
     __kmp_push_sync(global_tid, ct_reduce, loc, NULL, 0);
 #else
   if (__kmp_env_consistency_check)
     __kmp_push_sync(global_tid, ct_reduce, loc, NULL);
 #endif
 
+#if OMP_40_ENABLED
+  th = __kmp_thread_from_gtid(global_tid);
+  teams_swapped = __kmp_swap_teams_for_teams_reduction(th, &team, &task_state);
+#endif // OMP_40_ENABLED
+
   packed_reduction_method = __kmp_determine_reduction_method(
       loc, global_tid, num_vars, reduce_size, reduce_data, reduce_func, lck);
   __KMP_SET_REDUCTION_METHOD(global_tid, packed_reduction_method);
 
   if (packed_reduction_method == critical_reduce_block) {
 
     __kmp_enter_critical_section_reduce_block(loc, global_tid, lck);
     retval = 1;
if (__kmp_env_consistency_check) {
       }
     }
 
   } else {
 
     // should never reach this block
     KMP_ASSERT(0); // "unexpected method"
   }
+#if OMP_40_ENABLED
+  if (teams_swapped) {
+    __kmp_restore_swapped_teams(th, team, task_state);
+  }
+#endif
 
   KA_TRACE(10,
            ("__kmpc_reduce() exit: called T#%d: method %08x, returns %08x\n",
             global_tid, packed_reduction_method, retval));
 
   return retval;
 }
 
 /*!
 @ingroup SYNCHRONIZATION
 @param loc source location information
 @param global_tid global thread id.
 @param lck pointer to the unique lock data structure
 
 Finish the execution of a blocking reduce.
 The <tt>lck</tt> pointer must be the same as that used in the corresponding
 start function.
 */
 void __kmpc_end_reduce(ident_t *loc, kmp_int32 global_tid,
                        kmp_critical_name *lck) {
 
   PACKED_REDUCTION_METHOD_T packed_reduction_method;
+#if OMP_40_ENABLED
+  kmp_info_t *th;
+  kmp_team_t *team;
+  int teams_swapped = 0, task_state;
+#endif
 
   KA_TRACE(10, ("__kmpc_end_reduce() enter: called T#%d\n", global_tid));
 
+#if OMP_40_ENABLED
+  th = __kmp_thread_from_gtid(global_tid);
+  teams_swapped = __kmp_swap_teams_for_teams_reduction(th, &team, &task_state);
+#endif // OMP_40_ENABLED
+
   packed_reduction_method = __KMP_GET_REDUCTION_METHOD(global_tid);
 
   // this barrier should be visible to a customer and to the threading profile
   // tool (it's a terminating barrier on constructs if NOWAIT not specified)
 
   if (packed_reduction_method == critical_reduce_block) {
 
     __kmp_end_critical_section_reduce_block(loc, global_tid, lck);
#endif
     __kmp_end_split_barrier(UNPACK_REDUCTION_BARRIER(packed_reduction_method),
                             global_tid);
 
   } else {
 
     // should never reach this block
     KMP_ASSERT(0); // "unexpected method"
   }
+#if OMP_40_ENABLED
+  if (teams_swapped) {
+    __kmp_restore_swapped_teams(th, team, task_state);
+  }
+#endif
 
   if (__kmp_env_consistency_check)
     __kmp_pop_sync(global_tid, ct_reduce, loc);
 
   KA_TRACE(10, ("__kmpc_end_reduce() exit: called T#%d: method %08x\n",
                 global_tid, packed_reduction_method));
 
   return;

openmp/trunk/runtime/test/misc_bugs/teams-reduction.c

+// RUN: %libomp-compile-and-run
+//
+// The test checks the teams construct with reduction executed on the host.
+//
+
+#include <stdio.h>
+#include <omp.h>
+
+#include <stdint.h>
+
+#ifndef N_TEAMS
+#define N_TEAMS 4
+#endif
+#ifndef N_THR
+#define N_THR 3
+#endif
+
+// Internal library stuff to emulate compiler's code generation:
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+typedef struct {
+  int32_t reserved_1;
+  int32_t flags;
+  int32_t reserved_2;
+  int32_t reserved_3;
+  char const *psource;
+} ident_t;
+
+static ident_t dummy_loc = {0, 2, 0, 0, ";dummyFile;dummyFunc;0;0;;"};
+
+typedef int32_t kmp_critical_name[8];
+kmp_critical_name crit;
+
+int32_t __kmpc_global_thread_num(ident_t *);
+void __kmpc_push_num_teams(ident_t *, int32_t global_tid, int32_t num_teams,
+                           int32_t num_threads);
+void __kmpc_fork_teams(ident_t *, int32_t argc, void *microtask, ...);
+int32_t __kmpc_reduce(ident_t *, int32_t global_tid, int32_t num_vars,
+                      size_t reduce_size, void *reduce_data, void *reduce_func,
+                      kmp_critical_name *lck);
+void __kmpc_end_reduce(ident_t *, int32_t global_tid, kmp_critical_name *lck);
+
+#ifdef __cplusplus
+}
+#endif
+
+// Outlined entry point:
+void outlined(int32_t *gtid, int32_t *tid) {
+  int32_t ret = __kmpc_reduce(&dummy_loc, *gtid, 0, 0, NULL, NULL, &crit);
+  __kmpc_end_reduce(&dummy_loc, *gtid, &crit);
+}
+
+int main() {
+  int32_t th = __kmpc_global_thread_num(NULL); // registers initial thread
+  __kmpc_push_num_teams(&dummy_loc, th, N_TEAMS, N_THR);
+  __kmpc_fork_teams(&dummy_loc, 0, &outlined);
+
+  // Test did not hang -> passed!
+  printf("passed\n");
+  return 0;
+}