[PATCH] D82470: [OpenMP][IRBuilder] Support allocas in nested parallel regions

[Fady Ghanim via Phabricator via llvm-commits]

fghanim added a comment.

Thanks for the update. Just a couple of Nits, and a quick note



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:288
   struct OutlineInfo {
-    SmallVector<BasicBlock *, 32> Blocks;
     using PostOutlineCBTy = std::function<void(Function &)>;
----------------
Nit: isn't this supposed to be part of one of the other patches you split of this?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:433
 
-  Builder.SetInsertPoint(OuterFn->getEntryBlock().getFirstNonPHI());
-  AllocaInst *TIDAddr = Builder.CreateAlloca(Int32, nullptr, "tid.addr");
-  AllocaInst *ZeroAddr = Builder.CreateAlloca(Int32, nullptr, "zero.addr");
+  IRBuilder<> AllocaBuilder(AllocaIP.getBlock(), AllocaIP.getPoint());
+
----------------
Here and elsewhere: You seem to have forgotten to undo the changes introduced by using a separate `AllocaBuilder`?

Also, Nit: before using `AllocaIP`, either add an assert that it is not empty, or alternatively, add logic to just set it to entry block of `outerfn` if it is


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:490
   // of the outlined function.
-  InsertPointTy AllocaIP(PRegEntryBB,
-                         PRegEntryBB->getTerminator()->getIterator());
-  Builder.restoreIP(AllocaIP);
+  AllocaIP =
+      InsertPointTy(PRegEntryBB, PRegEntryBB->getTerminator()->getIterator());
----------------
NIT: I think it would be better to give each a different name, to avoid unnecessary confusion. maybe `OuterAllocaIP` and `innerAllocaIP`? Also, we don't overwrite the `outerAllocaIP` in case it's needed later?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82470