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

[Fady Ghanim via Phabricator via llvm-commits]

fghanim added a comment.

Great work. Thanks.

Also, please add the revision where this is used in clang as a child revision. It's a great validity check for this, and also showcases the intended way this is expected to be used. Up to this point I have been guessing the intent.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:24-52
+enum OpenMPSchedType {
+  /// Lower bound for default (unordered) versions.
+  OMP_sch_lower = 32,
+  OMP_sch_static_chunked = 33,
+  OMP_sch_static = 34,
+  OMP_sch_dynamic_chunked = 35,
+  OMP_sch_guided_chunked = 36,
----------------
Thanks for moving these over.

Where do you use them in this patch? I cannot find the usage.

In any case, and as pointed out by @jdoerfert , these belong in `OpenMPKInds.def`, not here. But in all likelyhood we already have them there. Please verify whether they already exist or not, and if they do use what we already have.
if not then move them over and use a similar style (look for `OMP_IDENT_FLAG` as an example to see what I mean).
If you don't use them here, then please do so in a separate patch - I suspect it may turn out to be a bit involved.




================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:787
+    ArrayRef<BodyGenCallbackTy> SectionCBs, PrivatizeCallbackTy PrivCB,
+    FinalizeCallbackTy FiniCB, bool IsCancellable, bool IsNoWait) {
+  if (!updateToLocation(Loc))
----------------
Where do you call the `FiniCB` callback? I cannot find it anywhere.

if not, then you probably should. this is where finalization code is done (e.g. destructors call + any other cleanup code)

At this stage, I am not sure if we need a single `Finicb` for the entire thing, or one per `SectionCB`.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:819-824
+    auto *ForBodyBB = BasicBlock::Create(M.getContext(),
+                                         "omp.sections.inner.for.body", CurFn);
+    auto *ForExitBB = BasicBlock::Create(M.getContext(),
+                                         "omp.sections.inner.for.exit", CurFn);
+    auto *ForIncBB =
+        BasicBlock::Create(M.getContext(), "omp.sections.inner.for.inc", CurFn);
----------------
Maybe you missed this. A little diagram for the intended CFG structure as a comment would be very helpful.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:834
+    Instruction *UBRef = Builder.CreateLoad(UB);
+    Value *cmpRef = Builder.CreateICmpSLE(IVRef, UBRef, "cmp");
+    Builder.CreateCondBr(cmpRef, ForBodyBB, ForExitBB);
----------------
NIT: Give the CmpRef instruction a name more descriptive than "cmp" - It will make reading the resultant IR much easier :)


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:844-855
+    // each section in emitted as a switch case
+    // Iterate through all sections and emit a switch construct:
+    // switch (IV) {
+    //   case 0:
+    //     <SectionStmt[0]>;
+    //     break;
+    // ...
----------------
regarding my the request for a CFG diagram ; You can add to the comment above -in similar style- to show how the BBs created for the loop interact with this/ look like.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:862
+      Builder.SetInsertPoint(CaseBB);
+      SectionCB(InsertPointTy(), Builder.saveIP(), *SwitchExitBB);
+      CaseNumber++;
----------------
I think from the perspective of C/C++, each one of these cases is a scope that should have a finalization block that we use for cleanup code. as such I am not sure if having one block for all cleanup is the right thing to do. This is why I am asking about the revision for usage in clang.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:897
+
+  CreateForLoopCB();
+
----------------
Do you use this callback anywhere else? if not, inline this as well.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1152-1155
+  EXPECT_EQ(F, OutlinedFn);
+  EXPECT_FALSE(verifyModule(*M, &errs()));
+  EXPECT_EQ(OutlinedFn->arg_size(), 1U);
+  EXPECT_EQ(OutlinedFn->getBasicBlockList().size(), 8U);
----------------
I don't think these checks are adequate. Generally, what you should be testing for is the generator's "defining features" - for lack of better name. examples for things you should be testing
- your loop - do we have the proper bounds, does it have the correct checks, is it generating the correct branches to name a few. 
- are we generating correct sections, and are they terminated appropriately (i.e. do they branch to correct/corresponding FiniCB) etc.
- is the `switchInst` being created correct (correct number of cases, branches to correct blocks for each case, etc.)
- all omp runtime calls (are we generating all correct runtime calls, correct arg., correct locations, etc.)
- any other thing you think is important and needs to be regularly tested


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1158
+  ASSERT_EQ(NumBodiesGenerated, 2U);
+  ASSERT_EQ(NumFiniCBCalls, 0U);
+}
----------------
OK, so you are not calling FiniCB anywhere. This is wrong. please modify the implementation to call the proper `FiniCB`, and change this test to reflect that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89671