[LLVM][OpenMP] Adding support for OpenMP sections construct in OpenMPIRBuilder

[AMDChirag]

Summary

This patch adds section support in the OpenMP IRBuilder module, along with a test for the same.

[fghanim]

Thank you for working on this!

Could you post the review for the clang usage of this. I think it is important that this is included so we can understand how this is going to be used. Please add it as a child revision to this one so it is not lost in the comments.

Also, Please change the title from [flang][openmp].... to [llvm][openmp]....; While the biggest beneficiary from the OMPBuilder is flang, it is part of the LLVM project, not flang. Also, this way the proper filters are triggered for people who are interested in this. :)

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

48–50

This is a duplicate of BodyGenCallbackTy. Please remove and replace uses with that instead.

llvm/include/llvm/IR/IRBuilder.h

1652–1658 ↗

(On Diff #298942)

Few points:
1- Do you use this outside the OMPBuilder? if no, then please move it there as a private method
2- NIT: it is preferable that this goes in separate patch - if we decide to keep it here

1655 ↗

(On Diff #298942)

Is LVal going to always be constant? if yes, then please add checks to make sure of that.
If not, then I believe it is better to not have this entire creator to begin with. since Alloca needs to go into entry block, and non-constant LVal may be created at a later BB.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

852–859

All allocaInst created here need to go into the entry block of the function (or in this case, the OMP outlined region). Please insert these into the entry block.

Until we figure out where to keep allocaIP, pass it from the Frontend as a parameter to createSections. Clang is properly set up to always have the correct one at all times, and I have a strong suspicion that we can do the same on the OMP-IR side of things.

865–871

A little diagram comment for the structure of the intended CFG is highly appreciated and super helpful. :)

866–871

Here and elsewhere, for BasicBlock::Create() - insertAfter combos : i think the insertafter is redundant. You can specify the function a basic block (BB) belongs to as part of its creation, and if a BB to insert before is not specified, it will be automatically added as last block in list.
if you expect there may be a BB after InsertBB, then I think it's worthwhile to get that block and use that as the insertBefore BB.

FWIW, ordering in the blocklist is not super important, so you may just add it wherever you want

918

Just wanna make sure I am not missing something; Are using this callback anywhere other than here? if not, Why not just inline its contents in place of this callback?

[AMDChirag]

@fghanim Hello and thank you!

I have updated the title.

Apologies, I am not well versed with this, but could you explain what does LVal being always constant mean in this context?
And also, what sort of checks do I add for that?

[fghanim]

In D89671#2340761, @AMDChirag wrote:

I have updated the title.

Great .. Thanks!

Apologies, I am not well versed with this, but could you explain what does LVal being always constant mean in this context?

No worries. we are all at different stages of learning.
Welcome to LLVM. :)

I saw that you always use createLVal with a contant value for initialization. So my Questions was are we always going to initialize the LVal we created with a constant value or not? because if not, we have the problem I mentioned in the original comment.

And also, what sort of checks do I add for that?

a simple assert with a "nice descriptive message" or any other more appropriate check you want is good enough;

[AMDChirag]

This comment was removed by AMDChirag.

[AMDChirag]

This comment was removed by AMDChirag.

[AMDChirag]

Amended the original commit, applying the fixes:

• removed BGenCallbackTy (was a duplicate of BodyGenCallbackTy)
• removed CreateLVal function
• all allocas for sections internal variables are grouped together
• removed InsertAfter() calls since they were redundant
• inlined the CreateSwitchCB callback

[jdoerfert]

Some drive-by comments. I will assume @fghanim continues the review, feel free to not wait on me.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

54

This should go into OMPKinds.def or the tablegen file.

502

Commit things like this without review as NFC changes.

523

Is nowait has no doxygen comment above. I would prefer IsNowait as spelling.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

851

Nit: CurIP (to match the other spelling above).

933

Comments, here and elsewhere, should be proper sentences.

960

No braces.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

1471

FWIW, tt never will be, that is a front-end task.

[fghanim]

Great work. Thanks.

Also, please add the revision where this is used in clang as a child revision. It's a great validity check for this, and also showcases the intended way this is expected to be used. Up to this point I have been guessing the intent.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

26–54

Thanks for moving these over.

Where do you use them in this patch? I cannot find the usage.

In any case, and as pointed out by @jdoerfert , these belong in OpenMPKInds.def, not here. But in all likelyhood we already have them there. Please verify whether they already exist or not, and if they do use what we already have.
if not then move them over and use a similar style (look for OMP_IDENT_FLAG as an example to see what I mean).
If you don't use them here, then please do so in a separate patch - I suspect it may turn out to be a bit involved.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

841

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.

873–878

Maybe you missed this. A little diagram for the intended CFG structure as a comment would be very helpful.

888

NIT: Give the CmpRef instruction a name more descriptive than "cmp" - It will make reading the resultant IR much easier :)

898–909

regarding my the request for a CFG diagram ; You can add to the comment above -in similar style- to show how the BBs created for the loop interact with this/ look like.

916

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.

951

Do you use this callback anywhere else? if not, inline this as well.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

1488–1491

I don't think these checks are adequate. Generally, what you should be testing for is the generator's "defining features" - for lack of better name. examples for things you should be testing

• your loop - do we have the proper bounds, does it have the correct checks, is it generating the correct branches to name a few.
• are we generating correct sections, and are they terminated appropriately (i.e. do they branch to correct/corresponding FiniCB) etc.
• is the switchInst being created correct (correct number of cases, branches to correct blocks for each case, etc.)
• all omp runtime calls (are we generating all correct runtime calls, correct arg., correct locations, etc.)
• any other thing you think is important and needs to be regularly tested

1494

OK, so you are not calling FiniCB anywhere. This is wrong. please modify the implementation to call the proper FiniCB, and change this test to reflect that.

[AMDChirag]

Applied some review changes

Moved the scheduler type enum to OpenMPKinds.def, inlined
CreateForLoopCB callback.
Some review changes are still being worked on.
The clang side of change will have a new revision created soon as the
child of this revision (D89671).

[jdoerfert]

The diff is against the previous version, not against a base version in LLVM. I usually squash all changes into a single commit but there are other ways too.

[AMDChirag]

squashed the commits

[AMDChirag]

Minor corrections in naming

[AMDChirag]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

841

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.

[AMDChirag]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

916

So, for this, the basic blocks of cases should have the finiCB callback called too?
Something like

auto CaseBB = CreateBB();
SetInsertPoint(CaseBB);
SectionCB(...);
FiniCB(...);

[Meinersbur]

One idea: Now with D90830 landed, CreateSections could be implemented by calling that (with TripCount == Number of sections), then call "CreateWorkshareLoop" (the same that we would use to implement #pragma omp for) to it, this would allow us to share the implementations of worksharing-loop and sections.

CreateWorkshareLoop does not exist yet, we might refactor this code once it exists.

[fghanim]

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

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

841

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

916

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

[AMDChirag]

Added FiniCB usage and Control Flow Graph

[fghanim]

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

523

Nit: add Doxygen documentation for IsNoWait

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

852

you only need to use the stack for outlined regions - for inlined regions (e.g. section/sections) you don't.

966

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.

[AMDChirag]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

966

So, to confirm,
There should be another function called CreateSection which should be called from within CreateSections for each of the sections, 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.

[fghanim]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

966

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

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

1488–1491

Don't forget about this. please :)

[AMDChirag]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

966

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 😊

[AMDChirag]

Updated code, test case, fixed some issues

createSections() now uses EmitOMPInlinedRegion().
The code is updated to use proper function names. Comments have been updated as well.
Test case is a lot more thorough and checks for loop variables, barrier, and loop functions.
Fixed a bug that caused crash when an external variable (outside #pragma omp section) was used.

The clang side updates will be pushed soon (D91054).

[AMDChirag]

Updated information for StoredBodyGenCallbackTy

[jdoerfert]

In D89671#2383849, @Meinersbur wrote:

One idea: Now with D90830 landed, CreateSections could be implemented by calling that (with TripCount == Number of sections), then call "CreateWorkshareLoop" (the same that we would use to implement #pragma omp for) to it, this would allow us to share the implementations of worksharing-loop and sections.

CreateWorkshareLoop does not exist yet, we might refactor this code once it exists.

What happend to using this?

[AMDChirag]

This comment was removed by AMDChirag.

[AMDChirag]

@jdoerfert Ouch. Totally forgot about that, apologies, on it.
Would it be fine/better to have the change for that in a separate patch though? Or must it be a part of this patch itself?

[jdoerfert]

In D89671#2484859, @AMDChirag wrote:

@jdoerfert Ouch. Totally forgot about that, apologies, on it.
Would it be fine/better to have the change for that in a separate patch though? Or must it be a part of this patch itself?

It should replace a lot of logic in this, right? I would prefer to use the existing functionality right away.

[fghanim]

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h

83–86 ↗

(On Diff #313305)

This and the OMPKinds.def part: split into their own patch, and they can go separately.
LGTM for both parts.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

966

Sorry for the delayed response - I got busy with a paper and a new project and this slipped through. :)

nop. Clang will always recursively call emitSection from within emitSections(), once per section. I have another diagram for you in the other comment about the recent changes.

Thanks a bunch, btw, for helping me so much 😊

Sure, my pleasure :)

968

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

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

1510

Nit: use OutlinedFn->getEntryBlock(). it's better.

[AMDChirag]

This comment was removed by AMDChirag.

[AMDChirag]

This comment was removed by AMDChirag.

[AMDChirag]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

968

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?

[jdoerfert]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

968

Must EmitOMPInlinedRegion be modified to allow nullptr for those parameters?

Probably. Feel free to modify the API where necessary.

[AMDChirag]

Added the use of createCanonicalLoop and createStaticWorkshareLoop.
Created OMP.createSection for handling the section's code generation.
Section callbacks are no longer a part of createSections.
Modified EmitOMPInlinedRegion to be able to handle nullptr for EntryCall/ExitCall parameters.
Removed changes from OMPConstants.hpp and OMPKinds.def to, originally, move to separate differential, but now are not required since loop creation is handled with createCanonicalLoop and createStaticWorkshareLoop.
OMP.createSections no longer uses EmitOMPInlinedRegion.

[AMDChirag]

Address a comment.

[AMDChirag]

removed the accidentally added test statement

[AMDChirag]

Currently cancellation clause is failing to work, within sections directive. Working on it.

[AMDChirag]

Added IsCancellable parameter to EmitOMPInlinedRegion for sections construct.
Even after which, the cancel construct is causing an assertion failure.
For now, this is marked as an expected failure and the FIXME comment is available.
The clang test case (marked XFAIL) is available in frontend patch - D91054.

[AMDChirag]

Fixed cancel codegen crash, updated test case.

[AMDChirag]

Removed the mistakenly added utils file (common.py)

[jdoerfert]

I only have small nits, nothing major. @fghanim, is this OK with you, if so, please accept?

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

100

Newline. Reference BodyGenCallBackTy for argument explanation.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

848

Prefer early exits

860

No backtracking, and similar schemes, if you can just remember the blocks you are looking for when you create them.

866

957

same comments as above, early exists & remember the blocks rather than looking for them

[fghanim]

LGTM from me

[AMDChirag]

Incorporated comments.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

860

Could you tell me how do I remember the blocks at creation point? The blocks are not created in the same function so I believe the only way would be to pass the blocks as function arguments?
Am I missing something?

[AMDChirag]

@jdoerfert Anything else to do here? Can this be merged?

[AMDChirag]

Rebased to HEAD of main branch

[AMDChirag]

Corrected rebase

[MaskRay]

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

1508

I fixed a -Wsign-compare here. Will be great to test the change with clang:)

[AMDChirag]

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

1508

Thank you very much for the fix and the kind words :)

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/Support/Allocator.h"
 #include <forward_list>
 
 namespace llvm {
 class CanonicalLoopInfo;
 
 /// An interface to create LLVM-IR for OpenMP directives.
 ///
 /// Each OpenMP directive has a corresponding public generator method.
 class OpenMPIRBuilder {
 public:
   /// Create a new OpenMPIRBuilder operating on the given module \p M. This will
   /// not have an effect on \p M (see initialize).
   OpenMPIRBuilder(Module &M) : M(M), Builder(M.getContext()) {}
 
   /// Initialize the internal state, this will put structures types and
   /// potentially other helpers into the underlying module. Must be called
   /// before any other method and only once!
   void initialize();
 
   /// Finalize the underlying module, e.g., by outlining regions.
   void finalize();
 
   /// Add attributes known for \p FnID to \p Fn.
   void addAttributes(omp::RuntimeFunction FnID, Function &Fn);
 
   /// Type used throughout for insertion points.
   using InsertPointTy = IRBuilder<>::InsertPoint;
 
   /// Callback type for variable finalization (think destructors).
   ///
   /// \param CodeGenIP is the insertion point at which the finalization code
   ///                  should be placed.
   ///
   /// A finalize callback knows about all objects that need finalization, e.g.
   /// destruction, when the scope of the currently generated construct is left
   /// at the time, and location, the callback is invoked.
   using FinalizeCallbackTy = std::function<void(InsertPointTy CodeGenIP)>;
 
   struct FinalizationInfo {
     /// The finalization callback provided by the last in-flight invocation of
     /// createXXXX for the directive of kind DK.
     FinalizeCallbackTy FiniCB;
public:
   /// \param CodeGenIP is the insertion point at which the body code should be
   ///                  placed.
   /// \param ContinuationBB is the basic block target to leave the body.
   ///
   /// Note that all blocks pointed to by the arguments have terminators.
   using BodyGenCallbackTy =
       function_ref<void(InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
                         BasicBlock &ContinuationBB)>;
+  // This is created primarily for sections construct as llvm::function_ref
+  // are not storable
+  using StorableBodyGenCallbackTy =
+      std::function<void(InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+                         BasicBlock &ContinuationBB)>;
 
   /// Callback type for loop body code generation.
   ///
   /// \param CodeGenIP is the insertion point where the loop's body code must be
   ///                  placed. This will be a dedicated BasicBlock with a
   ///                  conditional branch from the loop condition check and
   ///                  terminated with an unconditional branch to the loop
   ///                  latch.
public:
   /// Generator for '#omp critical'
   ///
   /// \param Loc The insert and source location description.
   /// \param BodyGenCB Callback that will generate the region body code.
   /// \param FiniCB Callback to finalize variable copies.
   /// \param CriticalName name of the lock used by the critical directive
   /// \param HintInst Hint Instruction for hint clause associated with critical
   ///
   /// \returns The insertion position *after* the master.
   InsertPointTy createCritical(const LocationDescription &Loc,
                                BodyGenCallbackTy BodyGenCB,
                                FinalizeCallbackTy FiniCB,
                                StringRef CriticalName, Value *HintInst);
 
+  /// Generator for '#omp sections'
+  ///
+  /// \param Loc The insert and source location description.
+  /// \param AllocaIP The insertion points to be used for alloca instructions.
+  /// \param SectionCBs Callbacks that will generate body of each section.
+  /// \param PrivCB Callback to copy a given variable (think copy constructor).
+  /// \param FiniCB Callback to finalize variable copies.
+  /// \param IsCancellable Flag to indicate a cancellable parallel region.
+  /// \param IsNowait If true, barrier - to ensure all sections are executed
+  /// before moving forward will not be generated.
+  /// \returns The insertion position *after* the sections.
+  InsertPointTy createSections(const LocationDescription &Loc,
+                               InsertPointTy AllocaIP,
+                               ArrayRef<StorableBodyGenCallbackTy> SectionCBs,
+                               PrivatizeCallbackTy PrivCB,
+                               FinalizeCallbackTy FiniCB, bool IsCancellable,
+                               bool IsNowait);
+
+  /// Generator for '#omp section'
+  ///
+  /// \param Loc The insert and source location description.
+  /// \param BodyGenCB Callback that will generate the region body code.
+  /// \param FiniCB Callback to finalize variable copies.
+  /// \returns The insertion position *after* the section.
+  InsertPointTy createSection(const LocationDescription &Loc,
+                              BodyGenCallbackTy BodyGenCB,
+                              FinalizeCallbackTy FiniCB);
+
   /// Generate conditional branch and relevant BasicBlocks through which private
   /// threads copy the 'copyin' variables from Master copy to threadprivate
   /// copies.
   ///
   /// \param IP insertion block for copyin conditional
   /// \param MasterVarPtr a pointer to the master variable
   /// \param PrivateVarPtr a pointer to the threadprivate variable
   /// \param IntPtrTy Pointer size type
private:
   /// \param EntryCall Call to the entry OMP Runtime Function
   /// \param ExitCall Call to the ending OMP Runtime Function
   /// \param BodyGenCB Body code generation callback.
   /// \param FiniCB Finalization Callback. Will be called when finalizing region
   /// \param Conditional indicate if the entry call result will be used
   ///        to evaluate a conditional of whether a thread will execute
   ///        body code or not.
   /// \param HasFinalize indicate if the directive will require finalization
-  ///         and has a finalization callback in the stack that
-  /// should        be called.
-  ///
+  ///        and has a finalization callback in the stack that
+  ///        should be called.
+  /// \param IsCancellable if HasFinalize is set to true, indicate if the
+  ///        the directive should be cancellable.
   /// \return The insertion point after the region
 
   InsertPointTy
   EmitOMPInlinedRegion(omp::Directive OMPD, Instruction *EntryCall,
                        Instruction *ExitCall, BodyGenCallbackTy BodyGenCB,
                        FinalizeCallbackTy FiniCB, bool Conditional = false,
-                       bool HasFinalize = true);
+                       bool HasFinalize = true, bool IsCancellable = false);
 
   /// Get the platform-specific name separator.
   /// \param Parts different parts of the final name that needs separation
   /// \param FirstSeparator First separator used between the initial two
   ///        parts of the name.
   /// \param Separator separator used between all of the rest consecutive
   ///        parts of the name
   static std::string getNameWithSeparators(ArrayRef<StringRef> Parts,

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

 }
 
 void OpenMPIRBuilder::createTaskyield(const LocationDescription &Loc) {
   if (!updateToLocation(Loc))
     return;
   emitTaskyieldImpl(Loc);
 }
 
+// TODO: Handle privatisation callbacks
+// FIXME: Sections construct with cancellation directive currently fails,
+// because EmitOMPRegionBody() removes any existing block terminator
+// which in turn causes FiniCB callback to fail in FinalizeOMPRegion()
+// as no terminator instruction (br) exists for the cancel block (<xyz>.cncl)
+// the test case for this is in clang/test/OpenMP/sections_cancel_irbuilder.cpp
+OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createSections(
+    const LocationDescription &Loc, InsertPointTy AllocaIP,
+    ArrayRef<StorableBodyGenCallbackTy> SectionCBs, PrivatizeCallbackTy PrivCB,
+    FinalizeCallbackTy FiniCB, bool IsCancellable, bool IsNowait) {
+  if (!updateToLocation(Loc))
+    return Loc.IP;
+
+  FinalizationStack.push_back({FiniCB, OMPD_sections, IsCancellable});
+
+  // Each section in emitted as a switch case
+  // Each finalization callback is handled from clang.EmitOMPSectionDirective()
+  // -> OMP.createSection() which generates the IR for each section
+  // Iterate through all sections and emit a switch construct:
+  // switch (IV) {
+  //   case 0:
+  //     <SectionStmt[0]>;
+  //     break;
+  // ...
+  //   case <NumSection> - 1:
+  //     <SectionStmt[<NumSection> - 1]>;
+  //     break;
+  // }
+  // ...
+  // section_loop.after:
+  // <FiniCB>;
+  auto LoopBodyGenCB = [&](InsertPointTy CodeGenIP, Value *IndVar) {
+    auto *CurFn = CodeGenIP.getBlock()->getParent();
+    auto *ForIncBB = CodeGenIP.getBlock()->getSingleSuccessor();
+    SwitchInst *SwitchStmt = Builder.CreateSwitch(IndVar, ForIncBB);
+    Builder.restoreIP(CodeGenIP);
+    unsigned CaseNumber = 0;
+    for (auto SectionCB : SectionCBs) {
+      auto *CaseBB = BasicBlock::Create(M.getContext(),
+                                        "omp_section_loop.body.case", CurFn);
+      SwitchStmt->addCase(Builder.getInt32(CaseNumber), CaseBB);
+      Builder.SetInsertPoint(CaseBB);
+      SectionCB(InsertPointTy(), Builder.saveIP(), *ForIncBB);
+      CaseNumber++;
+    }
+    // remove the existing terminator from body BB since there can be no
+    // terminators after switch/case
+    CodeGenIP.getBlock()->getTerminator()->eraseFromParent();
+  };
+  // Loop body ends here
+  // LowerBound, UpperBound, and STride for createCanonicalLoop
+  Type *I32Ty = Type::getInt32Ty(M.getContext());
+  Value *LB = ConstantInt::get(I32Ty, 0);
+  Value *UB = ConstantInt::get(I32Ty, SectionCBs.size());
+  Value *ST = ConstantInt::get(I32Ty, 1);
+  llvm::CanonicalLoopInfo *LoopInfo = createCanonicalLoop(
+      Loc, LoopBodyGenCB, LB, UB, ST, true, false, AllocaIP, "section_loop");
+  LoopInfo = createStaticWorkshareLoop(Loc, LoopInfo, AllocaIP, true);
+  BasicBlock *LoopAfterBB = LoopInfo->getAfter();
+  Instruction *SplitPos = LoopAfterBB->getTerminator();
+  if (!isa_and_nonnull<BranchInst>(SplitPos))
+    SplitPos = new UnreachableInst(Builder.getContext(), LoopAfterBB);
+  // ExitBB after LoopAfterBB because LoopAfterBB is used for FinalizationCB,
+  // which requires a BB with branch
+  BasicBlock *ExitBB =
+      LoopAfterBB->splitBasicBlock(SplitPos, "omp_sections.end");
+  SplitPos->eraseFromParent();
+
+  // Apply the finalization callback in LoopAfterBB
+  auto FiniInfo = FinalizationStack.pop_back_val();
+  assert(FiniInfo.DK == OMPD_sections &&
+         "Unexpected finalization stack state!");
+  Builder.SetInsertPoint(LoopAfterBB->getTerminator());
+  FiniInfo.FiniCB(Builder.saveIP());
+  Builder.SetInsertPoint(ExitBB);
+
+  return Builder.saveIP();
+}
+
+OpenMPIRBuilder::InsertPointTy
+OpenMPIRBuilder::createSection(const LocationDescription &Loc,
+                               BodyGenCallbackTy BodyGenCB,
+                               FinalizeCallbackTy FiniCB) {
+  if (!updateToLocation(Loc))
+    return Loc.IP;
+
+  Directive OMPD = Directive::OMPD_sections;
+  // Since we are using Finalization Callback here, HasFinalize
+  // and IsCancellable have to be true
+  return EmitOMPInlinedRegion(OMPD, nullptr, nullptr, BodyGenCB, FiniCB,
+                              /*Conditional*/ false, /*hasFinalize*/ true,
+                              /*IsCancellable*/ true);
+}
+
 OpenMPIRBuilder::InsertPointTy
 OpenMPIRBuilder::createMaster(const LocationDescription &Loc,
                               BodyGenCallbackTy BodyGenCB,
                               FinalizeCallbackTy FiniCB) {
 
   if (!updateToLocation(Loc))
     return Loc.IP;
 
   Directive OMPD = Directive::OMPD_master;
   Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
   Value *Ident = getOrCreateIdent(SrcLocStr);
   Value *ThreadId = getOrCreateThreadID(Ident);
   Value *Args[] = {Ident, ThreadId};
 
   Function *EntryRTLFn = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_master);
   Instruction *EntryCall = Builder.CreateCall(EntryRTLFn, Args);
 
   Function *ExitRTLFn = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_end_master);
   Instruction *ExitCall = Builder.CreateCall(ExitRTLFn, Args);
 
   return EmitOMPInlinedRegion(OMPD, EntryCall, ExitCall, BodyGenCB, FiniCB,
                               /*Conditional*/ true, /*hasFinalize*/ true);
 }
 
 CanonicalLoopInfo *OpenMPIRBuilder::createLoopSkeleton(
     DebugLoc DL, Value *TripCount, Function *F, BasicBlock *PreInsertBefore,
     BasicBlock *PostInsertBefore, const Twine &Name) {
   Module *M = F->getParent();
   LLVMContext &Ctx = M->getContext();
   Type *IndVarTy = TripCount->getType();
 
   // Create the basic block structure.
   BasicBlock *Preheader =
       BasicBlock::Create(Ctx, "omp_" + Name + ".preheader", F, PreInsertBefore);
   BasicBlock *Header =
       BasicBlock::Create(Ctx, "omp_" + Name + ".header", F, PreInsertBefore);
   BasicBlock *Cond =
       BasicBlock::Create(Ctx, "omp_" + Name + ".cond", F, PreInsertBefore);
   BasicBlock *Body =
       BasicBlock::Create(Ctx, "omp_" + Name + ".body", F, PreInsertBefore);
   BasicBlock *Latch =
       BasicBlock::Create(Ctx, "omp_" + Name + ".inc", F, PostInsertBefore);
   BasicBlock *Exit =
       BasicBlock::Create(Ctx, "omp_" + Name + ".exit", F, PostInsertBefore);
   BasicBlock *After =
       BasicBlock::Create(Ctx, "omp_" + Name + ".after", F, PostInsertBefore);
OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createCritical(
 
   return EmitOMPInlinedRegion(OMPD, EntryCall, ExitCall, BodyGenCB, FiniCB,
                               /*Conditional*/ false, /*hasFinalize*/ true);
 }
 
 OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::EmitOMPInlinedRegion(
     Directive OMPD, Instruction *EntryCall, Instruction *ExitCall,
     BodyGenCallbackTy BodyGenCB, FinalizeCallbackTy FiniCB, bool Conditional,
-    bool HasFinalize) {
+    bool HasFinalize, bool IsCancellable) {
 
   if (HasFinalize)
-    FinalizationStack.push_back({FiniCB, OMPD, /*IsCancellable*/ false});
+    FinalizationStack.push_back({FiniCB, OMPD, IsCancellable});
 
   // Create inlined region's entry and body blocks, in preparation
   // for conditional creation
   BasicBlock *EntryBB = Builder.GetInsertBlock();
   Instruction *SplitPos = EntryBB->getTerminator();
   if (!isa_and_nonnull<BranchInst>(SplitPos))
     SplitPos = new UnreachableInst(Builder.getContext(), EntryBB);
   BasicBlock *ExitBB = EntryBB->splitBasicBlock(SplitPos, "omp_region.end");
if (!Conditional && SkipEmittingRegion) {
     Builder.SetInsertPoint(InsertBB);
   }
 
   return Builder.saveIP();
 }
 
 OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::emitCommonDirectiveEntry(
     Directive OMPD, Value *EntryCall, BasicBlock *ExitBB, bool Conditional) {
-
   // if nothing to do, Return current insertion point.
-  if (!Conditional)
+  if (!Conditional || !EntryCall)
     return Builder.saveIP();
 
   BasicBlock *EntryBB = Builder.GetInsertBlock();
   Value *CallBool = Builder.CreateIsNotNull(EntryCall);
   auto *ThenBB = BasicBlock::Create(M.getContext(), "omp_region.body");
   auto *UI = new UnreachableInst(Builder.getContext(), ThenBB);
 
   // Emit thenBB and set the Builder's insertion point there for
if (HasFinalize) {
 
     BasicBlock *FiniBB = FinIP.getBlock();
     Instruction *FiniBBTI = FiniBB->getTerminator();
 
     // set Builder IP for call creation
     Builder.SetInsertPoint(FiniBBTI);
   }
 
+  if (!ExitCall)
+    return Builder.saveIP();
+
   // place the Exitcall as last instruction before Finalization block terminator
   ExitCall->removeFromParent();
   Builder.Insert(ExitCall);
 
   return IRBuilder<>::InsertPoint(ExitCall->getParent(),
                                   ExitCall->getIterator());
 }
 

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

for (auto &FI : *ThenBB) {
     }
   }
   EXPECT_NE(SingleEndCI, nullptr);
   EXPECT_EQ(SingleEndCI->getNumArgOperands(), 2U);
   EXPECT_TRUE(isa<GlobalVariable>(SingleEndCI->getArgOperand(0)));
   EXPECT_EQ(SingleEndCI->getArgOperand(1), SingleEntryCI->getArgOperand(1));
 }
 
+TEST_F(OpenMPIRBuilderTest, CreateSections) {
+  using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+  using BodyGenCallbackTy = llvm::OpenMPIRBuilder::StorableBodyGenCallbackTy;
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+  F->setName("func");
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+  llvm::SmallVector<BodyGenCallbackTy, 4> SectionCBVector;
+  llvm::SmallVector<BasicBlock *, 4> CaseBBs;
+
+  BasicBlock *SwitchBBPtr = nullptr;
+  BasicBlock *FiniBBPtr =
+      nullptr; // is actually loop's increment BB from code's point of view
+  AllocaInst *PrivAI = nullptr;
+  SwitchInst *Switch = nullptr;
+
+  unsigned NumBodiesGenerated = 0;
+  unsigned NumFiniCBCalls = 0;
+  PrivAI = Builder.CreateAlloca(F->arg_begin()->getType());
+
+  auto FiniCB = [&](InsertPointTy IP) {
+    ++NumFiniCBCalls;
+    BasicBlock *IPBB = IP.getBlock();
+    EXPECT_NE(IPBB->end(), IP.getPoint());
+  };
+
+  auto SectionCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+                       BasicBlock &FiniBB) {
+    ++NumBodiesGenerated;
+    CaseBBs.push_back(CodeGenIP.getBlock());
+    SwitchBBPtr = CodeGenIP.getBlock()->getSinglePredecessor();
+    Builder.restoreIP(CodeGenIP);
+    Builder.CreateStore(F->arg_begin(), PrivAI);
+    Value *PrivLoad = Builder.CreateLoad(PrivAI, "local.alloca");
+    Builder.CreateICmpNE(F->arg_begin(), PrivLoad);
+    Builder.CreateBr(&FiniBB);
+    FiniBBPtr = &FiniBB;
+  };
+  auto PrivCB = [](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+                   llvm::Value &, llvm::Value &Val, llvm::Value *&ReplVal) {
+    // TODO: Privatization not implemented yet
+    return CodeGenIP;
+  };
+
+  SectionCBVector.push_back(SectionCB);
+  SectionCBVector.push_back(SectionCB);
+
+  IRBuilder<>::InsertPoint AllocaIP(&F->getEntryBlock(),
+                                    F->getEntryBlock().getFirstInsertionPt());
+  Builder.restoreIP(OMPBuilder.createSections(Loc, AllocaIP, SectionCBVector,
+                                              PrivCB, FiniCB, false, false));
+  Builder.CreateRetVoid(); // Required at the end of the function
+
+  EXPECT_NE(PrivAI, nullptr);
+  Function *OutlinedFn = PrivAI->getFunction();
+  EXPECT_EQ(F, OutlinedFn);
+  EXPECT_FALSE(verifyModule(*M, &errs()));
+  EXPECT_EQ(OutlinedFn->arg_size(), 1U);
+  EXPECT_EQ(OutlinedFn->getBasicBlockList().size(), 11);
+
+  BasicBlock *LoopPreheaderBB =
+      OutlinedFn->getEntryBlock().getSingleSuccessor();
+  // loop variables are 5 - lower bound, upper bound, stride, islastiter, and
+  // iterator/counter
+  bool found_for_init = false;
+  for (Instruction &Inst : *LoopPreheaderBB) {
+    if (isa<CallInst>(Inst)) {
+      if (cast<CallInst>(&Inst)->getCalledFunction()->getName() ==
+          "__kmpc_for_static_init_4u") {
+        found_for_init = true;
+      }
+    }
+  }
+  EXPECT_EQ(found_for_init, true);
+
+  // Switch BB's predecessor is loop condition BB, whose successor at index 1 is
+  // loop's exit BB
+  BasicBlock *ForExitBB =
+      SwitchBBPtr->getSinglePredecessor()->getTerminator()->getSuccessor(1);
+  bool found_for_exit = false;
+  bool found_barrier = false;
+  for (Instruction &Inst : *ForExitBB) {
+    if (isa<CallInst>(Inst)) {
+      if (cast<CallInst>(&Inst)->getCalledFunction()->getName() ==
+          "__kmpc_for_static_fini") {
+        found_for_exit = true;
+      }
+      if (cast<CallInst>(&Inst)->getCalledFunction()->getName() ==
+          "__kmpc_barrier") {
+        found_barrier = true;
+      }
+      if (found_for_exit && found_barrier)
+        break;
+    }
+  }
+  EXPECT_EQ(found_for_exit, true);
+  EXPECT_EQ(found_barrier, true);
+
+  EXPECT_NE(SwitchBBPtr, nullptr);
+  EXPECT_NE(SwitchBBPtr->getTerminator(), nullptr);
+  EXPECT_EQ(isa<SwitchInst>(SwitchBBPtr->getTerminator()), true);
+  Switch = cast<SwitchInst>(SwitchBBPtr->getTerminator());
+  EXPECT_EQ(Switch->getNumCases(), 2U);
+  EXPECT_EQ(Switch->getSuccessor(0), FiniBBPtr);
+
+  EXPECT_EQ(CaseBBs.size(), 2U);
+  for (auto *&CaseBB : CaseBBs) {
+    EXPECT_EQ(CaseBB->getParent(), OutlinedFn);
+    EXPECT_EQ(CaseBB->getSingleSuccessor(), FiniBBPtr);
+  }
+
+  ASSERT_EQ(NumBodiesGenerated, 2U);
+  ASSERT_EQ(NumFiniCBCalls, 1U);
+}
+
 } // namespace