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

[Fady Ghanim via Phabricator via llvm-commits]

fghanim added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:83-86
+enum class SchedType {
+#define OMP_SCHED_TYPE(Enum, Str, Value) Enum = Value,
+#include "llvm/Frontend/OpenMP/OMPKinds.def"
+};
----------------
This and the OMPKinds.def part: split into their own patch, and they can go separately.
LGTM for both parts.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:968
+      Builder.SetInsertPoint(CaseBB->getTerminator());
+      SectionFiniCBs[CaseNumber](Builder.saveIP());
+      CaseNumber++;
----------------
This won't work as you expect it to :)

What will happen based on this code is:
```
clang.emitparallel() 
  -->OMPBuilder.creatparallel( ..., bodyCB ,...) 
    --> BodyCB ()
      --> clang.emitSections() 
        --> OMPBuilder.createSections( ..., SectionCB[N], ...) 
         | --> SectionCBs[i]()
         | |  --> OMPBuilderCBHelpers::EmitOMPRegionBody() --> clang.emitStmt() ---> clang.emitSection() 
         | |    --> {use clang internals to emit the section body and correct finalization}
         |  --> sectionFiniCBs[i]() //a second round of finalizations 
```
IIRC, the second round of finalization will either assert or end up corrupting a cleanup stack inside clang (don't remember which).


As I said in the original comment on this. You don't have to get `CreateSection` implemented. I strongly think that the clang lit test (i.e. the test for the other patch) will work just fine without this. If it doesn't, we'll have to look into the reason why and if we need to implement `createSection` for it to work. -- i.e. we'll cross that bridge when we have to. :)


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:912
+         "Unexpected Directive for Finalization call!");
+  Fi.FiniCB(InsertPointTy(SwitchExitBB, SwitchExitBB->getFirstInsertionPt()));
+
----------------
AMDChirag wrote:
> fghanim wrote:
> > AMDChirag wrote:
> > > fghanim wrote:
> > > > What I meant by my comment; is that you will use the same finiCB multiple times - once per section. you should undo the last changes, However, no need to do add anything back, but based on your clang impl. this is not the place for it.
> > > > every `sections` directive has one or more `section` directive, and based on clang impl. that thing is generated separately.  The finalization block will be generated inside the `section` directive.
> > > > 
> > > > Implementation-wise; this translates to writing a `createSection` which can be implemented as an inlined region, look at `createCritical` for an example of how to do it. The clang side is also quite easy - again look at `emitOMPCritical` in clang for an example.
> > > > 
> > > > I am **not ** going to require that you write `createSection` along with its clang usage to accept this patch, **unless ** the clang lit test for `omp sections` doesn't work without it.
> > > So, to confirm,
> > > There should be another function called `CreateSection` which should be called from within `CreateSections` for each of the `section`s, and that would use the FiniCB callback?
> > > Something like
> > > ```
> > > CreateSections() {
> > > ...Init...
> > > for each section {
> > >     call CreateSection(SectionCB, FiniCB)
> > > }
> > > ```
> > > Where CreateSection will use the `EmitOMPInlinedRegion()` function?
> > > 
> > > Also, the clang lit test case for the clang side of things will be in the clang patch. I'll be creating the test.
> > Basically yes. to be extra specific :D
> > The way clang and the OMPBuilder will end up handling this
> > ```
> > clang.emitparallel() 
> >   -->OMPBuilder.creatparallel( ..., bodyCB ,...) 
> >     --> BodyCB ()
> >       --> clang.emitSections() 
> >         --> OMPBuilder.createSections( ..., SectionCB[N], ...) 
> >           --> SectionCB[i]()
> >             --> clang.emitSection() 
> >               --> OMPBuilder.createsection()
> >                  --> {this will contain the body of section[i] and finalization code for it - essentially the same finiCB used once per section).
> > ```
> Okay, I am working on integration with `EmitOMPInlinedRegion()`.
> 
> Regarding the flow though, I have a doubt.
> With OMPIRBuilder work, the `clang.EmitSections()` function will not be used since it does scaffolding work which is taken care of by `OMPIRBuilder`.
> From my understanding, the flow of functions would be something like:
> ```
> Clang.EmitOMPSectionsDirective() -> OMP.CreateSections() -> (possibly) OMP.EmitOMPInlinedRegion()
> ```
> where the `bodygen` callback will be declared in `OMP.CreateSections()` and `SectionCBVector` will be declared in `Clang.EmitOMPSectionsDirective()` and used in the `bodygen` callback.
> 
> Is that correct or am I missing something?
> 
> Thanks a bunch, btw, for helping me so much 😊
Sorry for the delayed response - I got busy with a paper and a new project and this slipped through. :)

nop. Clang will always recursively call `emitSection` from within `emitSections()`, once per section. I have another diagram for you in the other comment about the recent changes.

>> Thanks a bunch, btw, for helping me so much 😊
Sure, my pleasure :)


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1510
+
+  BasicBlock *EntryBB = &OutlinedFn->getBasicBlockList().front();
+  // loop variables are 5 - lower bound, upper bound, stride, islastiter, and
----------------
Nit: use `OutlinedFn->getEntryBlock()`. it's better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89671