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

[Fady Ghanim via Phabricator via Openmp-commits]

fghanim added inline comments.


================
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());
+
----------------
jdoerfert wrote:
> fghanim wrote:
> > jdoerfert wrote:
> > > fghanim wrote:
> > > > 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
> > > > Here and elsewhere: You seem to have forgotten to undo the changes introduced by using a separate AllocaBuilder?
> > > 
> > > I don't get this. These changes are on purpose. 
> > Oh, my bad. I assumed that since we now pass `allocaip` to communicate where is the insertion point, using `builder` the way we used to is sufficient, and this was leftover code. So now what purpose does `AllocaBuilder` serve?
> 1) No switching the IP back and forth in one builder all the time.
> 2) Showing by the name of the builder directly where the instructions will be created.
>No switching the IP back and forth in one builder all the time.
- Is this about something you expect in the future? because unless I am missing something, and looking at it side by side, I don't see any less setting /reseting of IPs going on, except in the new version we to set the debug info IP in `allocabuilder` in addition to in `builder`.
- we don't use either `AllocaIP` after returning from `bodyCB`
- Even if we did, both `bodyCB`, and `FiniCB` have IPGuards inside them.
- Also, since IRBuilders do `insertbefore` an IP, both `AllocaIP` should be up-to-date except if the iterator for an IP gets invalidated, the risk of which is sort of proportional to increase in number of IRbuilders.

>Showing by the name of the builder directly where the instructions will be created.
Wouldn't a comment be better? 
Personally, I find it confusing when something called `AllocaBuilder`, is being used to emit loads and stores.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82470