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

[Chirag Khandelwal via Phabricator via llvm-commits]

AMDChirag added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:912
+         "Unexpected Directive for Finalization call!");
+  Fi.FiniCB(InsertPointTy(SwitchExitBB, SwitchExitBB->getFirstInsertionPt()));
+
----------------
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 😊


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89671