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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
