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