[Openmp-commits] [PATCH] D45326: [OpenMP] [CUDA plugin] Add support for teams reduction via scratchpad

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

Some comments inline, mostly minor things.



================
Comment at: libomptarget/plugins/cuda/src/rtl.cpp:64-67
   SPMD, // constructors, destructors,
         // combined constructs (`teams distribute parallel for [simd]`)
   GENERIC, // everything else
   NONE
----------------
Shouldn't you be explicitly assigning values to this `enum`? Currently, it's not obvious which values they will hold.

(And I think the names should not all be upper case (except `SPMD`), but only the first character...)


================
Comment at: libomptarget/plugins/cuda/src/rtl.cpp:496
 
-      if (ExecModeVal < 0 || ExecModeVal > 1) {
-        DP("Error wrong exec_mode value specified in cubin file: %d\n",
-           ExecModeVal);
+      if (CP.ExecutionMode < 0 || CP.ExecutionMode > 1) {
+        DP("Error wrong target kernel computation properties value specified in"
----------------
You should be using `SPMD` and `>= None` here.


================
Comment at: libomptarget/plugins/cuda/src/rtl.cpp:638-641
+    if (KernelInfo->ExecutionMode == GENERIC) {
+      // Leave room for the master warp which will be added below.
+      cudaThreadsPerBlock -= DeviceInfo.WarpSize[device_id];
+    }
----------------
I think this shouldn't be in this patch?


================
Comment at: libomptarget/plugins/cuda/src/rtl.cpp:709-712
+#ifdef OMPTARGET_DEBUG
+    if (Scratchpad == NULL)
+      DP("Failed to allocate reduction scratchpad\n");
+#endif
----------------
Maybe error out completely?


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D45326