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

[Fady Ghanim via Phabricator via llvm-commits]

fghanim added a comment.

I second @Meinersbur comment. Reducing duplication is always better :)



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:787
+    ArrayRef<BodyGenCallbackTy> SectionCBs, PrivatizeCallbackTy PrivCB,
+    FinalizeCallbackTy FiniCB, bool IsCancellable, bool IsNoWait) {
+  if (!updateToLocation(Loc))
----------------
AMDChirag wrote:
> fghanim wrote:
> > 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`.
> Hi @fghanim 
> Could you please tell me how the multiple FiniCB thing will work? Will there be separate definitions for the FiniCBs or the single definition (which is right now) will be used multiple times (= the number of section CBs)?
> The frontend work has been added - revision D91054.
The only time where you will require multiple different FiniCB, is if the code to emit cleanup code for different sections is different, which -unless i am missing something- i don't think is the case - for Clang at least. So it's very likely that Having one finiCB defined and reusing it for each region is going to work


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:862
+      Builder.SetInsertPoint(CaseBB);
+      SectionCB(InsertPointTy(), Builder.saveIP(), *SwitchExitBB);
+      CaseNumber++;
----------------
AMDChirag wrote:
> fghanim wrote:
> > 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.
> So, for this, the basic blocks of cases should have the finiCB callback called too?
> Something like
> ```
> auto CaseBB = CreateBB();
> SetInsertPoint(CaseBB);
> SectionCB(...);
> FiniCB(...);
> ```
yes, but you may need to create a block (e.g. `FiniBB`) reachable from `caseBB`, and pass it to `FiniCB` to emit code there.

you can check `createMaster`, `createSingle`, `createCritical` for an example


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89671