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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:968
+      Builder.SetInsertPoint(CaseBB->getTerminator());
+      SectionFiniCBs[CaseNumber](Builder.saveIP());
+      CaseNumber++;
----------------
AMDChirag wrote:
> fghanim wrote:
> > 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. :)
> Okay, I think (and hope) I get it now.
> One question though, when using `EmitOMPInlinedRegion` for each section, what will the arguments for `EntryCall` and `ExitCall` parameters be? Since, as of right now, no Entry/Exit functions are used?
> Must `EmitOMPInlinedRegion` be modified to allow `nullptr` for those parameters?
> Must EmitOMPInlinedRegion be modified to allow nullptr for those parameters?

Probably. Feel free to modify the API where necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89671