[PATCH] D74372: [OpenMP][IRBuilder] Perform finalization (incl. outlining) late

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 7 inline comments as done.
jdoerfert added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:113
+  if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder())
+    OMPBuilder->finalize();
 }
----------------
fghanim wrote:
> Does this mean that `finalize()` is being called twice everytime, here and `~OpenMPIRBuilder()`?
> if yes, is there a reason why you want that?
It's called twice now, not that it matters. The basic rule is, call it at least once before you finalize the module. I'll remove the destructor for now to make it explicit only.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:275
+  /// Get a handle for a new region that will be outlined later.
+  OutlineInfo &getNewOutlineInfo() {
+    OutlineInfos.push_back({});
----------------
JonChesterfield wrote:
> Calling getNewOutlineInfo will invalidate the references previously returned. That's not a bug in this patch but looks like it'll be easy to get wrong in future.
> 
> Perhaps the backing store should be a linked list, such that push_back doesn't invalidate any existing references?
The address of these objects is not stable but without that requirement we don't need a list. We make this function take a `OutlineInfo` object that we than move into the vector if that is preferred.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:114
+
+    // Add some known attributes to the outlined function.
+    Function *OutlinedFn = Extractor.extractCodeRegion(CEAC);
----------------
rogfer01 wrote:
> This comment seems misplaced now.
Agreed, will be moved.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:587
+      CallInst *CI = cast<CallInst>(OutlinedFn.user_back());
+      CI->getParent()->setName("omp_parallel");
+      Builder.SetInsertPoint(CI);
----------------
JonChesterfield wrote:
> s/parallel/outlined?
this is in the `omp parallel` callback so "parallel" is still fine. 


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:654-655
+    Instruction *PreFiniTI = PRegPreFiniBB->getTerminator();
+    OuterFn->dump();
+    PreFiniTI->dump();
+    assert(PreFiniTI->getNumSuccessors() == 1 &&
----------------
rogfer01 wrote:
> I think you forgot to wrap these in a `LLVM_DEBUG`
I simplify forgot the remove them. Thx for noticing.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:663
 
-  return AfterIP;
+    InsertPointTy ExitIP(UI->getParent(), UI->getParent()->end());
+    UI->eraseFromParent();
----------------
rogfer01 wrote:
> This used to be called `AfterIP`. Probably the exact name is not that important but the unit tests still use `AfterIP` so I'd suggest to keep both names in sync.
I'll call it `AfterIP` again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74372