[OpenMP] Avoid reading uninitialized parallel level values

[jdoerfert]

Summary

In a last minute change request for a2dbfb6b72db we introduced a
read of the uninitialized parallel level value in SPMD-mode.
We go back to initializing the array early and checking for an
adjusted level.

Found by the miniqmc unit tests:

https://cdash.qmcpack.org/CDash/viewTest.php?onlyfailed&buildid=203434

[JonChesterfield]

This fix looks sound. D95976 itself is quite complicated, don't have a handle on what that is doing yet.

[ggeorgakoudis]

Does this work correctly with a nested parallel region in SPMD mode? I think the zeroing-out of the parallel-level is needed for this case.

[jdoerfert]

In D101123#2712248, @ggeorgakoudis wrote:

Does this work correctly with a nested parallel region in SPMD mode? I think the zeroing-out of the parallel-level is needed for this case.

The parallel level in SPMD mode can never be zero. It is 1 in the beginning and then can only be higher as we enter serialized parallel regions (Inc/Dec level in *_serialized_parallel).

[ggeorgakoudis]

In D101123#2712461, @jdoerfert wrote:

In D101123#2712248, @ggeorgakoudis wrote:

Does this work correctly with a nested parallel region in SPMD mode? I think the zeroing-out of the parallel-level is needed for this case.

The parallel level in SPMD mode can never be zero. It is 1 in the beginning and then can only be higher as we enter serialized parallel regions (Inc/Dec level in *_serialized_parallel).

I don't think it's going to enter the serialized parallel execution because the parallelLevel is not incremented in the parallel call. Perhaps we need to init parallelLevel in spmd_init to 0, if that doesn't cause any problems in other parts of the runtime, and leave the handling of the parallelLevel inside the parallel call. We should also add a test for nested parallel regions in SPMD mode too.

openmp/libomptarget/deviceRTLs/common/src/omptarget.cu

EXTERN void __kmpc_spmd_kernel_init(int ThreadLimit,
                                     int16_t RequiresOMPRuntime) {
   PRINT0(LD_IO, "call to __kmpc_spmd_kernel_init\n");
 
   setExecutionParameters(Spmd, RequiresOMPRuntime ? RuntimeInitialized
                                                   : RuntimeUninitialized);
   int threadId = GetThreadIdInBlock();
   if (threadId == 0) {
     usedSlotIdx = __kmpc_impl_smid() % MAX_SM;
+    parallelLevel[0] =
+        1 + (GetNumberOfThreadsInBlock() > 1 ? OMP_ACTIVE_PARALLEL_LEVEL : 0);
+  } else if (GetLaneId() == 0) {
+    parallelLevel[GetWarpId()] =
+        1 + (GetNumberOfThreadsInBlock() > 1 ? OMP_ACTIVE_PARALLEL_LEVEL : 0);
   }
   if (!RequiresOMPRuntime) {
     // Runtime is not required - exit.
     __kmpc_impl_syncthreads();
     return;
   }
 
   //

openmp/libomptarget/deviceRTLs/common/src/parallel.cu

 // parallel interface
 ////////////////////////////////////////////////////////////////////////////////
 
 EXTERN void __kmpc_parallel_51(kmp_Ident *ident, kmp_int32 global_tid,
                                kmp_int32 if_expr, kmp_int32 num_threads,
                                int proc_bind, void *fn, void *wrapper_fn,
                                void **args, size_t nargs) {
 
-  // Handle the serialized case first, same for SPMD/non-SPMD.
-  // TODO: Add UNLIKELY to optimize?
-  bool InParallelRegion = (__kmpc_parallel_level(ident, global_tid) > 0);
+  // Handle the serialized case first, same for SPMD/non-SPMD except that in
+  // SPMD mode we already incremented the parallel level counter, account for
+  // that.
+  bool InParallelRegion =
+      (__kmpc_parallel_level(ident, global_tid) > __kmpc_is_spmd_exec_mode());
   if (!if_expr || InParallelRegion) {
     __kmpc_serialized_parallel(ident, global_tid);
     __kmp_invoke_microtask(global_tid, 0, fn, args, nargs);
     __kmpc_end_serialized_parallel(ident, global_tid);
-
     return;
   }
 
   if (__kmpc_is_spmd_exec_mode()) {
-    // Increment parallel level for SPMD warps.
-    if (GetLaneId() == 0)
-      parallelLevel[GetWarpId()] =
-          1 + (GetNumberOfThreadsInBlock() > 1 ? OMP_ACTIVE_PARALLEL_LEVEL : 0);
-    // TODO: Is that synchronization correct/needed? Can only using a memory
-    // fence ensure consistency?
-    __kmpc_impl_syncthreads();
-
     __kmp_invoke_microtask(global_tid, 0, fn, args, nargs);
-
-    // Decrement (zero out) parallel level for SPMD warps.
-    if (GetLaneId() == 0)
-      parallelLevel[GetWarpId()] = 0;
     return;
   }
 
   // Handle the num_threads clause.
   if (num_threads != -1)
     __kmpc_push_num_threads(ident, global_tid, num_threads);
 
   __kmpc_kernel_prepare_parallel((void *)wrapper_fn);