[Openmp-commits] [openmp] 17330a3 - [OpenMP] Avoid reading uninitialized parallel level values

[Johannes Doerfert via Openmp-commits]

Author: Johannes Doerfert
Date: 2021-04-23T11:21:58-05:00
New Revision: 17330a3cb13aed9743f3f60eb8310c06aa34f1ca

URL: https://github.com/llvm/llvm-project/commit/17330a3cb13aed9743f3f60eb8310c06aa34f1ca
DIFF: https://github.com/llvm/llvm-project/commit/17330a3cb13aed9743f3f60eb8310c06aa34f1ca.diff

LOG: [OpenMP] Avoid reading uninitialized parallel level values

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

Reviewed By: JonChesterfield

Differential Revision: https://reviews.llvm.org/D101123

Added: 
    

Modified: 
    openmp/libomptarget/deviceRTLs/common/src/omptarget.cu
    openmp/libomptarget/deviceRTLs/common/src/parallel.cu

Removed: 
    


################################################################################
diff  --git a/openmp/libomptarget/deviceRTLs/common/src/omptarget.cu b/openmp/libomptarget/deviceRTLs/common/src/omptarget.cu
index 297bd01fcbf8a..e19d67affc2b7 100644
--- a/openmp/libomptarget/deviceRTLs/common/src/omptarget.cu
+++ b/openmp/libomptarget/deviceRTLs/common/src/omptarget.cu
@@ -88,6 +88,11 @@ EXTERN void __kmpc_spmd_kernel_init(int ThreadLimit,
   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.

diff  --git a/openmp/libomptarget/deviceRTLs/common/src/parallel.cu b/openmp/libomptarget/deviceRTLs/common/src/parallel.cu
index 1377de6773c67..29a6db85d172c 100644
--- a/openmp/libomptarget/deviceRTLs/common/src/parallel.cu
+++ b/openmp/libomptarget/deviceRTLs/common/src/parallel.cu
@@ -289,31 +289,20 @@ EXTERN void __kmpc_parallel_51(kmp_Ident *ident, kmp_int32 global_tid,
                                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;
   }