[PATCH] D131830: Clang Support for taskwait nowait clause

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:6056
+                            DepWaitTaskArgs);
+      }
 
----------------
Can we avoid the duplication, always make the array 7 long, and use the ArrayRef substring capability to pass in either 6 or 7 elements? 

Conceptually something like this:
```
Fill all
emitRuntimeCall(..., ArrayRef(DepWaitTaskArgs, OpenMP >= 51 ? 7: 6)
```

On second thought, why do we need to ever emit the old runtime call at all? Always emit the new one.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:6075
+                                M, OMPRTL___kmpc_omp_taskwait),
+                            Args);
+      }
----------------
Same as above. 


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:80
   /// not have an effect on \p M (see initialize).
-  OpenMPIRBuilder(Module &M) : M(M), Builder(M.getContext()) {}
+  OpenMPIRBuilder(Module &M, int OpenMPVersion = 40)
+      : M(M), Builder(M.getContext()), OpenMPVersion(OpenMPVersion) {}
----------------
Not sure a default is wise here. Users should pick. And we might want to have a "latest" constant somewhere, just set to 99.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:643
+  void createTaskwait(const LocationDescription &Loc,
+                      bool HasNoTaskwaitClause = false);
 
----------------
The clause is not NoTaskwait, it's nowait, right? Why not `HasNowaitClause`?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1273
+        getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_omp_taskwait), Args);
+  }
 }
----------------
Same as above, new version only is sufficient.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2347
            "schedule type does not support user-defined chunk sizes");
-    [[fallthrough]];
+    LLVM_FALLTHROUGH;
   case OMPScheduleType::BaseDynamicChunked:
----------------
Unrelated and not even sure which one we should use nowadays. Please split this off.


================
Comment at: openmp/runtime/src/kmp.h:3985
+                                            kmp_depend_info_t *noalias_dep_list,
+                                            kmp_int32 have_no_wait = false);
+
----------------
No default arguments here please.


================
Comment at: openmp/runtime/src/kmp_taskdeps.cpp:890
+                                 kmp_depend_info_t *noalias_dep_list,
+                                 kmp_int32 have_no_wait) {
+  KA_TRACE(10, ("__kmpc_omp_taskwait_deps(enter): T#%d loc=%p nowait#%d\n",
----------------
Implement the old version by calling the new version with no_wait false. I assume you copied the original code, this should have indicated a problem (wrt. code duplication).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131830/new/

https://reviews.llvm.org/D131830