[PATCH] D135695: [OMPIRBuilder] Support depend clause for task construct

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:210
 
+enum class OMPRTLDependenceKindTy {
+  OMPDepIn = 0x01,
----------------
psoni2628 wrote:
> domada wrote:
> > Do we need definition of `OMPRTLDependenceKindTy ` and `OpenMPDependKind `? It looks like these two class enums are overlapping.
> `OMPRTLDependenceKindTy` is to be used with the `OMPRTL___kmpc_omp_task_with_deps` runtime function to encode what kind of dependency a given clause is, whereas `OpenMPDependKind` is for the MLIR to encode what kind of dependency a given clause is. You could map them to the same thing, but IMO, it is better to keep them separate. `llvm::OpenMPIRBuilder::OpenMPDependKind::OMP_DEPEND_out` and `llvm::OpenMPIRBuilder::OpenMPDependKind::OMP_DEPEND_inout` happen to map to the same thing, `OMPRTLDependenceKindTy::OMPDepInOut`, which is what the `translateDependencyKind` function does, but I don't think it makes much sense to map them to the same thing at the MLIR level. IMO, they should be kept distinct.
I doubt we want duplication. It doesn't buy as anything and introduces confusion.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:644
+  enum OpenMPDependKind {
+    OMP_DEPEND_in,
+    OMP_DEPEND_out,
----------------
psoni2628 wrote:
> There is some code duplication going on here. For example, this enum is basically copied from `clang/include/clang/Basic/OpenMPKinds.def`. I tried extracting this enum into `llvm/include/llvm/Frontend/OpenMP/OMPConstants.h` so that Clang can use it, but it seems that the `OPENMP_DEPEND_KIND` macro is complex and does not lend itself to a simple extraction. The macro is redefined in many different files. Any suggestions here would be appreciated.
I don't understand the problem. Define all depend kinds similar to proc bind here https://github.com/llvm/llvm-project/blob/25162418c60475185cf29b5336015ca2f52ce3eb/llvm/include/llvm/Frontend/OpenMP/OMPKinds.def#L1052 . Then use that to create the enum class in OMPConstants.h and use that enum class as well as the macro everywhere, including clang.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1497
+      Value *DepArray =
+          Builder.CreateAlloca(DepArrayTy, nullptr, ".dep.arr.addr");
+
----------------
Is the insert point set to the right place for sure?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1532
+           llvm::ConstantPointerNull::get(Type::getInt8PtrTy(M.getContext()))});
+
+    } else {
----------------
Generally, llvm:: inside llvm/ if we open the namespace anyway.


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

https://reviews.llvm.org/D135695