[PATCH] D89671: [Flang][OpenMP] Adding support for OpenMP sections construct in OpenMPIRBuilder

[Fady Ghanim via Phabricator via llvm-commits]

fghanim added a reviewer: fghanim.
fghanim added a comment.

Thank you for working on this!

Could you post the review for the clang usage of this. I think it is important that this is included so we can understand how this is going to be used. Please add it as a child revision to this one so it is not lost in the comments.

Also, Please change the title from [flang][openmp].... to [llvm][openmp]....; While the biggest beneficiary from the `OMPBuilder` is flang, it is part of the LLVM project, not flang. Also, this way the proper filters are triggered for people who are interested in this. :)



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:75-77
+  using BGenCallbackTy =
+      std::function<void(InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+                         BasicBlock &ContinuationBB)>;
----------------
This is a duplicate of `BodyGenCallbackTy`. Please remove and replace uses with that instead.


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:1652-1658
+  Value *CreateLVal(Type *Ty, const Twine &Name = "", Value *Init = nullptr) {
+    Value *LVal = CreateAlloca(Ty, nullptr, Name);
+    if (Init) {
+      CreateStore(Init, LVal);
+    }
+    return LVal;
+  }
----------------
Few points:
1- Do you use this outside the `OMPBuilder`? if no, then please move it there as a private method
2- NIT: it is preferable that this goes in separate patch - if we decide to keep it here


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:1655
+    if (Init) {
+      CreateStore(Init, LVal);
+    }
----------------
Is `LVal` going to always be `constant`? if yes, then please add checks to make sure of that.
If not, then I believe it is better to not have this entire creator to begin with. since Alloca needs to go into entry block, and non-constant `LVal` may be created at a later BB.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:798-805
+  Value *LB = Builder.CreateLVal(Int32, "omp.sections.lb", Builder.getInt32(0));
+  ConstantInt *GlobalUBVal = Builder.getInt32(SectionCBs.size() - 1);
+  Value *UB = Builder.CreateLVal(Int32, ".omp.sections.ub", GlobalUBVal);
+  Value *ST =
+      Builder.CreateLVal(Int32, ".omp.sections.st.", Builder.getInt32(1));
+  Value *IL =
+      Builder.CreateLVal(Int32, ".omp.sections.il.", Builder.getInt32(0));
----------------
All allocaInst created here need to go into the entry block of the function (or in this case, the OMP outlined region). Please insert these into the entry block.

Until we figure out where to keep `allocaIP`, pass it from the Frontend as a parameter to `createSections`. Clang is properly set up to always have the correct one at all times, and I have a strong suspicion that we can do the same on the OMP-IR side of things.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:811-817
+    // create new basic blocks for emitting the for loop
+    auto *ForBodyBB = BasicBlock::Create(M.getContext(), "omp.inner.for.body");
+    auto *ForExitBB = BasicBlock::Create(M.getContext(), "omp.inner.for.exit");
+    auto *ForIncBB = BasicBlock::Create(M.getContext(), "omp.inner.for.inc");
+    CurFn->getBasicBlockList().insertAfter(InsertBB->getIterator(), ForIncBB);
+    CurFn->getBasicBlockList().insertAfter(InsertBB->getIterator(), ForBodyBB);
+    CurFn->getBasicBlockList().insertAfter(InsertBB->getIterator(), ForExitBB);
----------------
A little diagram comment for the structure of the intended CFG is highly appreciated and super helpful. :)



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:812-817
+    auto *ForBodyBB = BasicBlock::Create(M.getContext(), "omp.inner.for.body");
+    auto *ForExitBB = BasicBlock::Create(M.getContext(), "omp.inner.for.exit");
+    auto *ForIncBB = BasicBlock::Create(M.getContext(), "omp.inner.for.inc");
+    CurFn->getBasicBlockList().insertAfter(InsertBB->getIterator(), ForIncBB);
+    CurFn->getBasicBlockList().insertAfter(InsertBB->getIterator(), ForBodyBB);
+    CurFn->getBasicBlockList().insertAfter(InsertBB->getIterator(), ForExitBB);
----------------
Here and elsewhere, for BasicBlock::Create() - insertAfter combos : i think the insertafter is redundant. You can specify the function a basic block (BB) belongs to as part of its creation, and if a BB to insert before is not specified, it will be automatically added as last block in list.
if you expect there may be a BB after InsertBB, then I think it's worthwhile to get that block and use that as the insertBefore BB.

FWIW, ordering in the blocklist is not super important, so you may just add it wherever you want


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:864
+    Builder.SetInsertPoint(ForBodyBB);
+    CreateSwitchCB();
+    Builder.CreateBr(ForIncBB);
----------------
Just wanna make sure I am not missing something; Are using this callback anywhere other than here? if not, Why not just inline its contents in place of this callback?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89671