[Openmp-commits] [PATCH] D101123: [OpenMP] Avoid reading uninitialized parallel level values

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert created this revision.
jdoerfert added reviewers: ggeorgakoudis, ye-luo, tianshilei1992.
Herald added subscribers: jfb, guansong, yaxunl.
Herald added a reviewer: bollu.
jdoerfert requested review of this revision.
Herald added a subscriber: sstefan1.
Herald added a project: OpenMP.

In a last minute change request for a2dbfb6b72db <https://reviews.llvm.org/rGa2dbfb6b72db19ed851464160ef7539b50d43894> 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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101123

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


Index: openmp/libomptarget/deviceRTLs/common/src/parallel.cu
===================================================================
--- openmp/libomptarget/deviceRTLs/common/src/parallel.cu
+++ openmp/libomptarget/deviceRTLs/common/src/parallel.cu
@@ -289,31 +289,20 @@
                                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;
   }
 
Index: openmp/libomptarget/deviceRTLs/common/src/omptarget.cu
===================================================================
--- openmp/libomptarget/deviceRTLs/common/src/omptarget.cu
+++ openmp/libomptarget/deviceRTLs/common/src/omptarget.cu
@@ -88,6 +88,11 @@
   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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D101123.339836.patch
Type: text/x-patch
Size: 2380 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/openmp-commits/attachments/20210423/ee7c1657/attachment-0001.bin>