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

Chirag Khandelwal via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 24 22:41:13 PST 2020

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() {
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.

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list