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

[Prabhdeep Soni via Phabricator via llvm-commits]

psoni2628 added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:644
+  enum OpenMPDependKind {
+    OMP_DEPEND_in,
+    OMP_DEPEND_out,
----------------
jdoerfert wrote:
> 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.
Thanks for the suggestion! However, if we don't want to have 2 enums, one for the clause and one for the RTL function, as you mentioned in one of your other comments (quoted below), then I can just define a singular enum in `OMPConstants.h`. IMO, this is simpler.

> I doubt we want duplication. It doesn't buy as anything and introduces confusion.




================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1497
+      Value *DepArray =
+          Builder.CreateAlloca(DepArrayTy, nullptr, ".dep.arr.addr");
+
----------------
jdoerfert wrote:
> Is the insert point set to the right place for sure?
Looking more closely at it, I think this should be inserted before the `Then` of the `IfThenElse` for the `if` clause, but then the `OMPRTL___kmpc_omp_task_with_deps` call should still be in the `Then`.


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

https://reviews.llvm.org/D135695