[PATCH] D88706: [OpenMP][MLIR] WIP : Fix for nested parallel region

[Fady Ghanim via Phabricator via llvm-commits]

fghanim added a reviewer: fghanim.
fghanim added a comment.

After reading the patch for `master` you referred to, I don't understand why do we need the `OMPBuilder` to maintain the insertion point. As far as `master` is concerned, we will emit any alloca's contained inside its region into the entry block of the enclosing outlined region (e.g. innermost `parallel`).
FWIW, the `master` directive in clang already uses the `OMPBuilder` and just relies on `clang` to handle the insertion of any non-omp code (including alloca's). Is there a reason why a similar approach wouldn't work here?

If this is indeed needed for `master`, then please don't create extra `IRBuilder`s needlessly. As you mentioned, D82470 <https://reviews.llvm.org/D82470> had this exact approach implemented, and we ended up not going through with it.



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:724-727
+  LLVM_DEBUG({
+    for (Value *Output : Outputs)
+      LLVM_DEBUG(dbgs() << "Captured output: " << *Output << "\n");
+  });
----------------
These are unrelated to maintaining alloca insertion point. I don't recall back then whether these were removed completely, or added as part of other (2?) patches that were split off of this one.


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

https://reviews.llvm.org/D88706