[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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89671