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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Generally fine, I think. The alloca placement is the last bit that needs changing.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:644
+  enum OpenMPDependKind {
+    OMP_DEPEND_in,
+    OMP_DEPEND_out,
----------------
psoni2628 wrote:
> 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.
> 
> 
If these are all uses, I assume a fixed enum is good enough. 


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1441
+      Value *DepArray =
+          Builder.CreateAlloca(DepArrayTy, nullptr, ".dep.arr.addr");
+
----------------
Same question as before. How do you ensure the alloca is placed where you want it (which is the function entry block).


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1497
+      Value *DepArray =
+          Builder.CreateAlloca(DepArrayTy, nullptr, ".dep.arr.addr");
+
----------------
psoni2628 wrote:
> 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`.
The entire conditional here is a bad idea. We should pass the if-clause value to the runtime. This causes all sort of problems, but it's unrelated to this patch...


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

https://reviews.llvm.org/D135695