[OpenMP][OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

[fghanim]

Summary

Add support for Master and Critical directive in the OMPIRBuilder. Both make use of a new common interface for emitting inlined OMP regions called emitInlinedRegion which was added in this patch as well.

Also this patch modifies clang to use the new directives when -fopenmp-enable-irbuilder commandline option is passed.

[jdoerfert]

Some initial comments. Can we split it in two patches (master/critical)?

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

249

If there is no good reason agains it, these should go into the OMPKinds.def/OMPConstants.{h/cpp}. We need support for array types but that is not a problem.

258

A lot of the functions below should not be public. We should expose as little as possible, mainly the CreateDirective methods.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

641

You can just write OMPD_master below instead of OMPD.

651

Make sure the patch is clang formatted. See also: https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

845

Is this switch + if-cascade + callEntry/Exit really helpful?
I mean we can replace

`Instruction *EntryCall = CreateEntryCall(OMPD, Args);`

with

`Instruction *EntryCall = Builder.CreateCall(getOrCreateRuntimeFunction(OMPRTL___kmpc_omp_master), Args);`

right?

[fghanim]

I don't think there is enough in this patch to split it into two or three patches. The main part is EmitOMPInlinedRegion which does all the heavy lifting. At this point, both create Master & Critical are almost wrappers collecting arguments for EmitOMPInlinedRegion. So I really do not want to split them into multiple patches unless I have to.

As for the rest , I just wanted to provide my thinking when I did them. I'll update them if you think it's better to. let me know.

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

249

I tried to do that, but started to get compilation errors when I put them in OMPConstants.{h/cpp} . So I thought that given that these are used exactly once according to current implementation in clang (i.e. for 'critical name'). I'll do it as part of OMPBuilder for the time being.

258

I think the OMPbuilder should provide people writing llvm transformation passes (e.g. autoparallelizing) with a way to be able to emit correct OMP calls based on their needs.

If you have minor suggestions to make it better, I'd love to update this based on that.
Alternatively, I'll make them private for now, until we decide on something for that.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

641

Yes. I just feel this is a bit cleaner is all.

651

Will do

845

While I completely agree that the switch+if-cascade is not an elegant solution, It's helpful in enabling a generic way to the directive entry and exit calls. which is in turn helpful for EmitCommonDirectiveEntry and EmitCommonDirectiveExit.

FWIW, CreateEntryCall was used inside of EmitCommonDirectiveEntry, similar to how CreateExitCall is currently inside EmitCommonDirectiveExit. RTLFuncName enabled that. I only took it out because of Critical's Entry_Hint special case. I plan to put it back in, If critical is the only directive like that.

I am not too attached to it, but I think it's help-ing :)

[jdoerfert]

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

249

These are not good arguments. We have *all other types* defined in OMPKinds.def + OMPConstants.{h/cpp}, doing something different here just complicates the code.

258

It is futile to design based on some use cases we cannot describe properly. Once the need arises for public interfaces, the callType (inconsistent spelling!), the Entry/Exit call stuff, we can add it but not before.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

641

I argue it is harder to read the code this way (especially at the call site below). Though, I won't make the change a requirement.

845

What is EmitCommonDirectiveEntry and EmitCommonDirectiveExit ?

I don't see how it is helping to have emitEntry(createEntry(DIRECTIVE)) instead of emit(DIRECTIVE_ENTRY), especially since the former needs all this logic to map DIRECTIVE to DIRECTIVE_ENTRY. The latter shows you at the call site, e.g., in CreateCritical, exactly what runtime call is being created and at these call sites we already know exactly what runtime call we need.

You can still have some helper that puts entry/exit around some code, simply pass both the entry and exit runtime call ID instead of just the directive ID from which you would "compute" the entry/exit runtime call ID.

---

Long story short, if making things explicit minimizes code complexity along the way I'm all for it.

[jdoerfert]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

840

While I hope this code goes away, I should mention that we don't write assert(false && "...") but llvm_unreachable("...").

[fghanim]

• Adding array types to OMPKinds.def. Inlining runtime function calls generation. reformatting with clang format

[fghanim]

So I modified the patch based on your comments. I removed all the code that I intended for llvm pass writers for now.
I will look at it again later, figure something to do with the whole switch-if-cascade thing, and will introduce it in a separate patch. I think that would be better than to include it here.

[jdoerfert]

I added quite a bunch of inline comments again :( and some high level stuff below.

---

Commits need a message that explains the change.
Also the "Summary" should not be in the commit title.

---

This adds support for clang to build the directives via the OMPIRBuilder so we need to add tests for that. Probably "just" running existing codegen tests for the directives with the OMPIRBuidlder enabled.

clang/lib/CodeGen/CGStmtOpenMP.cpp

3005

Can you add an assertion that the branch has only a single successor.

3009

IPBBTI->eraseFromParent()

3020

Why the isSet check? If we need it, then it's missing in other places as well (I think).

3060

Can we do

llvm::Value *HintInst = nullptr;
if (Hint)
  ...

these three lines look ugly :(

3076

I somehow have the feeling we could have a "FiniCB" helper function as they seem to be always the same. A TODO mentioning that is also OK for now.

3099

Same here, that looks like the other "BodyGenCB". We should really make them helpers.

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

259

FiniCB param docu is missing.

295

Conditional

312

HasFinalize

331

Format: no newline, above the comments got broken

349

All methods need documentation of some kind. llvm:: is not needed here. It seems one method is declared twice.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

68

These comments that reference KmpCriticalNameTy do not help here. If you want, feel free to add comments that explain how the result looks in a generic way but only showing KmpCriticalNameTy seems counterproductive to me.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

674

Spelling: Fn

695

Spelling Conditional, HasFinalize

700

Outdated comment.

717

Nit: InsertPointTy FiniIP(FiniBB, FiniBB->getFirstInsertionPt());

721

Shouldn't we use the argument here?

728

Style: No braces

737

Conditional, here and elsewhere no llvm:: (and omp::)

739

Naming: ContPt or ContIP or ...

766

I usually have neither or both conditional branches in braces. Here we can opt for an early exist (preferred anyway) and get:

if (!Conditional)
  return Builder.SaveIP();
...

Some variables can then be moved past this early exit

790

extra parenthesis, and, actually, this condition can not happen (I think). There is always something at the end of the block or your get an error earlier.

800

ExitCall->moveBefore(IP), potentially with some IP = ... above.
We should specify what the builder insert position is after this call, maybe "unspecified"?

808

This can be a static helper and the name should be more descriptive, getName is very generic.

824

This seems to be too much trouble to get a StringRef from a Twine. Maybe start with a StringRef and avoid all of it?

836

Do we really want common linkage for "internal variables"? Internal or private would have been my first guess. Maybe the "internal" part is different than I expect it.

---

FWIW, the usual pattern is

if (!Elem.second)
  Elem.second = ...
assert(...)
return Elem.second;

[fghanim]

Commits need a message that explains the change.
Also the "Summary" should not be in the commit title.

Is this something for future commits or do you want me to change these?
If the latter, how do I do that?

This adds support for clang to build the directives via the OMPIRBuilder so we need to add tests for that. Probably "just" running existing codegen tests for the directives with the OMPIRBuidlder enabled.

You mean unit tests? or something else? If not unit tests, Where do I do that?

As for rest of comments, anything without response, is something I will do.

clang/lib/CodeGen/CGStmtOpenMP.cpp

3020

While I was writing the response I realized that I should've made it
assert((!AllocaIP.isSet() || AllocaInsertPt == AllocaIP.getpoint()) && "Inlined directives allocas should be emitted to entry block of the inner most containing outlined function";
or something less verbose.

Inlined directives should emit their allocas to the entry block of the containing outined function, right? For that reason, and since we don't store an alloca insertion point in the OMPBuilder, I pass an empty insertion point, and back up the current point, without changing it.
I was planning, in the future, to keep a copy of the most recent alloca insertion point which we set when we outline, and pass that for BodyCBs within inlined directives.

3060

np.

btw, the todo note is about the CGM.Int32Ty. Currently in clang it's CGM.IntTy. I'll expand on the todo

3076

I'll do a Todo for now. I am using a very similar FiniCB to the one you used for parallel. So it doesn't make sense to change these here and not that one with them.

3099

Same as prev.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

68

I was testing something, and I forgot to remove them when I was done.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

717

Nit?

721

"The argument" refers to?

[fghanim]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

808

This is taken from clang (I think CG_OMP) as is. I am keeping everything as is for now in case somebody decided to help and came across it being used somewhere else, they should recognize it immediately.

So for now, I suggest a todo. cos if I find that it's not used except with critical, I'll merge all 3 functions into 1 and be done with it.

824

Same as prev.

836

same as prev.

[jdoerfert]

In D72304#1815793, @fghanim wrote:

Commits need a message that explains the change.
Also the "Summary" should not be in the commit title.

Is this something for future commits or do you want me to change these?
If the latter, how do I do that?

Already for this one, at least the git commit.
You can edit this patch in phab (top right, edit revision) and you need to make sure to adjust the git commit before pushing.

This adds support for clang to build the directives via the OMPIRBuilder so we need to add tests for that. Probably "just" running existing codegen tests for the directives with the OMPIRBuidlder enabled.

You mean unit tests? or something else? If not unit tests, Where do I do that?

We can use lit test for that. The two files that you need to modify for sure are clang/test/OpenMP/critical_codegen.cpp and clang/test/OpenMP/master_codegen.cpp. You add run lines so that the code in clang is actually executed and modify the check lines appropriately.
I did split my commits into the llvm and clang part mostly, if you look at a clang patch which uses the OpenMPIRBuilder, you should see what I mean.

As for rest of comments, anything without response, is something I will do.

I did respond to the ones with.

clang/lib/CodeGen/CGStmtOpenMP.cpp

3020

I try not to make assumptions like "Inlined directives should emit their allocas to the entry block of the containing outined function, right" if we don't need to. If we do, the assert is probably a better way to go. I would actually say we want a helper that sets/resets all these things for *all* body functions. If the OMPBuilder doesn't want to set a new allocaip, it should provide an "invalid" location to indicate that. Now I finally see what you do here and why. Go with the assert for now, and make it !isSet() if there is no reason to complicate it further.

3076

Agreed. We need a helper that is used at both/all places.

3099

Again a TODO and we are good for now.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

68

I take it you'll remove them. Just saying because you commented ;)

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

717

Nit is often used to indicate a request that can be ignored silently (https://www.urbandictionary.com/define.php?term=nit)

721

Sorry. I mean passing hasFinalize instead of true

808

You can keep it a separate helper but I still don't see how it helps to keep this a member (instead of a static).

I also want the name change. My rational is simple: If we pick a name that is meaningful, people know what it does regardless of whether they know the version in clang.

824

Here, the argument that people recognize it doesn't work at all, it's a type. So please pass a StringRef if there is no other reason not to.

836

The code pattern is unrelated from "same as prev", I hope.

For the linkage, add a TODO to investigate if private linkage would work as well.

[fghanim]

I am working on a patch. In the mean time ;)

clang/lib/CodeGen/CGStmtOpenMP.cpp

3020

I think most (if not all) llvm passes that are concerned with allocas, expect them to be in the entry block, this is why I made this assumption. The only time that I can think of, where this will not be true is in case of a delayed outlining. In that case, the insertion block is going to be the "future" entry block of the "future" outlined function, and needs to be passed. Then I think it is also necessary to keep track of the insertion block somewhere inside the OMPBuilder.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

800

before this point, the builder is always set to insert before the terminator of the finalization block. when emitInlinedFunction is done, it sets it to ExitBB.end()

824

You are right. *That* argument doesn't work. However the argument that I found it this way in clang kinda does. Also the argument that it makes things much easier to move. ;)
I assume whoever did it this way had his/her good reason. I understand you may not think it is, but given that it's used a lot in clang for getting and creating internal OMP variables, you can't really judge without going over every case to make sure that what you want can be done in every case, as such I am against the change *NOW*. However, for the time being, I included a todo basically saying that.

836

The assert is only needed if Elem.second already exist. It's quite useless if it's not and we are creating it, right?
I'll change it to an if..else, and return it once.

critical name -which is what this is being used for now- has an external linkage, isn't it?
Also, as I said this is being used by many others all over CG_OMP to create OMP internal variables.
Maybe later we'll create different versions for different internal variables, but for now I'll add a todo anyway.

[fghanim]

Addressing reviewer comments

• Added regression tests
• styling
• adding asserts and todo where needed.

Also, Now EmitInlinedRegion will merge blocks where legal.

[jdoerfert]

LGTM with some minor things you should address before the merge.

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

90

Leftover comment.

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

272

Nit: typos in parameter name and comment, comments seems outdated.

305

Nit: Comment is not in sync with the Entry one.
Nit: spelling in the parameter name and format.

326

Nit: spelling param name and format.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

45

Leftover

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

695

This needs to be guarded by HasFinalize if the corresponding pop is.

749

I have the feeling the merging makes it harder without providing a benefit but I'm OK with it for now

759

omp is not a good name here.

791

HasFinalize

[fghanim]

Addressing reviewer's comments

• fixed styling and naming according to llvm conventions

[fghanim]

Thanks for reviewing this.

After I address the last two comments below, How do I merge with llvm using phab? I'd appreciate an llvm specific link if possible.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

45

what is this referring to?

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

749

more difficult in what way?

[jdoerfert]

Phab is not involved in the merge process, it just is a normal git push on top of the llmv-project.
Do you have commit rights already? If not I can commit this for you in a few days.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

45

You deleted an empty new line ;)

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

749

We need things like auto InsertBB = merged ? ExitPredBB : ExitBB; now for example.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

686

Nit: The else is not needed.

745

Nit: The else is not needed.

762

Nit: The else is not needed.

[jdoerfert]

The commit title has a stray { instead of [

[fghanim]

Addressing reviewer's comments regarding styling.

[fghanim]

No, I don't have commit privileges. I'd appreciate if you'd commit this for me. Thanks :)

[jdoerfert]

I tried to push this today but the build failed:

/data/src/llvm-project/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:252:36: error: use of undeclared identifier 'BumpPtrAllocator'
  StringMap<AssertingVH<Constant>, BumpPtrAllocator> InternalVars;
                                   ^
/data/src/llvm-project/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:832:10: error: no viable conversion from returned value of type 'llvm::StringRef' to function return type 'std::string' (aka 'basic_string<char>')
  return OS.str();
         ^~~~~~~~

Can you look into that please?

[fghanim]

Thanks for trying.

It compiles fine with me. How can I reproduce the problems you're getting? is there a way to pack what you have and send it to me so I can have a look?

Also, a second question, how trustworthy is harbormaster when it says something is buildable?

[jdoerfert]

In D72304#1846115, @fghanim wrote:

Thanks for trying.

It compiles fine with me. How can I reproduce the problems you're getting? is there a way to pack what you have and send it to me so I can have a look?

Did you rebase onto the newest llvm master?

Also, a second question, how trustworthy is harbormaster when it says something is buildable?

Honestly, I do not know.

[fghanim]

Everything is fine!
I just cloned llvm from git hub, added this revision with arc patch. then:
cmake -G"Unix Makefiles" -DLLVM_ENABLE_PROJECTS="clang;openmp" -DCMAKE_BUILD_TYPE=Release -DLLVM_OPTIMIZED_TABLEGEN=1 -DLLVM_USE_LINKER=gold ../llvm-project/llvm
make , make check-llvm-unittest, make check-clang-openmp

all of it works fine and I got no errors whatsoever. Let me know what you think.

[fghanim]

ping

[jdoerfert]

Reverted in 9dcfc7cd64abb301124cafaa95661b76a1fc5032 because of breakages and warnings:
http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/22899

[fghanim]

Addressing errors/warnings from commit build

[fghanim]

ping

[kadircet]

this breaks under asan http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/38597/steps/check-clang%20asan/logs/stdio

[fghanim]

fixing unused variable.

[fghanim]

Remove unnecessary preservation of builder insertion point in emitCommonDirectiveExit

[jdoerfert]

I did run clang-format on the OMPIRBuilder files and replaced tabs with spaces. I think you need to teach your editor to use spaces in the future.

[MaskRay]

test/CodeGen/opt-record-1.c triggers an assertion failure.

[jdoerfert]

In D72304#1877676, @MaskRay wrote:

test/CodeGen/opt-record-1.c triggers an assertion failure.

I hope the macro reordering in rGa0236de7a927 did the trick.

[fhahn]

It looks like this patch (or the other related changes committed recently) may cause ASan failures running Transforms/OpenMP/gtid.ll and Transforms/OpenMP/parallel_deletion.ll http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6960/consoleFull

It would be great if you could take a look and possibly revert the change if it takes some time to investigate.

[jdoerfert]

In D72304#1877855, @fhahn wrote:

It looks like this patch (or the other related changes committed recently) may cause ASan failures running Transforms/OpenMP/gtid.ll and Transforms/OpenMP/parallel_deletion.ll http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6960/consoleFull

It would be great if you could take a look and possibly revert the change if it takes some time to investigate.

Should already be fixed with rGa0236de7a927.

I wait for http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6961/ to come back.

clang/lib/CodeGen/CGStmtOpenMP.cpp

CGF.CGM.getOpenMPRuntime().emitParallelCall(CGF, S.getBeginLoc(), OutlinedFn,
                                               CapturedVars, IfCond);
 }
 
 static void emitEmptyBoundParameters(CodeGenFunction &,
                                      const OMPExecutableDirective &,
                                      llvm::SmallVectorImpl<llvm::Value *> &) {}
 
 void CodeGenFunction::EmitOMPParallelDirective(const OMPParallelDirective &S) {
-
   if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) {
     // Check if we have any if clause associated with the directive.
     llvm::Value *IfCond = nullptr;
     if (const auto *C = S.getSingleClause<OMPIfClause>())
       IfCond = EmitScalarExpr(C->getCondition(),
                               /*IgnoreResultAssign=*/true);
 
     llvm::Value *NumThreads = nullptr;
static void emitMaster(CodeGenFunction &CGF, const OMPExecutableDirective &S) {
   auto &&CodeGen = [&S](CodeGenFunction &CGF, PrePostActionTy &Action) {
     Action.Enter(CGF);
     CGF.EmitStmt(S.getInnermostCapturedStmt()->getCapturedStmt());
   };
   CGF.CGM.getOpenMPRuntime().emitMasterRegion(CGF, CodeGen, S.getBeginLoc());
 }
 
 void CodeGenFunction::EmitOMPMasterDirective(const OMPMasterDirective &S) {
+  if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) {
+    using InsertPointTy = llvm::OpenMPIRBuilder::InsertPointTy;
+
+    const Stmt *MasterRegionBodyStmt =
+        S.getInnermostCapturedStmt()->getCapturedStmt();
+    auto FiniCB = [this](InsertPointTy IP) {
+      CGBuilderTy::InsertPointGuard IPG(Builder);
+      assert(IP.getBlock()->end() != IP.getPoint() &&
+             "OpenMP IR Builder should cause terminated block!");
+
+      llvm::BasicBlock *IPBB = IP.getBlock();
+      llvm::BranchInst *IPBBTI =
+          llvm::dyn_cast<llvm::BranchInst>(IPBB->getTerminator());
+      llvm::BasicBlock *DestBB = IPBBTI->getSuccessor(0);
+
+      // erase and replace with cleanup branch.
+      IPBB->getTerminator()->eraseFromParent();
+      Builder.SetInsertPoint(IPBB);
+      CodeGenFunction::JumpDest Dest = getJumpDestInCurrentScope(DestBB);
+      EmitBranchThroughCleanup(Dest);
+    };
+
+    auto BodyGenCB = [MasterRegionBodyStmt, this](InsertPointTy AllocaIP,
+                                                  InsertPointTy CodeGenIP,
+                                                  llvm::BasicBlock &FiniBB) {
+      auto OldAllocaIP = AllocaInsertPt;
+      if (AllocaIP.isSet())
+        AllocaInsertPt = &*AllocaIP.getPoint();
+      auto OldReturnBlock = ReturnBlock;
+      ReturnBlock = getJumpDestInCurrentScope(&FiniBB);
+
+      llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
+      if (llvm::Instruction *CodeGenIPBBTI = CodeGenIPBB->getTerminator())
+        CodeGenIPBBTI->eraseFromParent();
+
+      Builder.SetInsertPoint(CodeGenIPBB);
+
+      EmitStmt(MasterRegionBodyStmt);
+
+      Builder.CreateBr(&FiniBB);
+
+      AllocaInsertPt = OldAllocaIP;
+      ReturnBlock = OldReturnBlock;
+    };
+
+    Builder.restoreIP(OMPBuilder->CreateMaster(Builder, BodyGenCB, FiniCB));
+
+    return;
+  }
   OMPLexicalScope Scope(*this, S, OMPD_unknown);
   emitMaster(*this, S);
 }
 
 void CodeGenFunction::EmitOMPCriticalDirective(const OMPCriticalDirective &S) {
+  if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) {
+    using InsertPointTy = llvm::OpenMPIRBuilder::InsertPointTy;
+
+    const Stmt *CriticalRegionBodyStmt =
+        S.getInnermostCapturedStmt()->getCapturedStmt();
+    const Expr *Hint = nullptr;
+    if (const auto *HintClause = S.getSingleClause<OMPHintClause>())
+      Hint = HintClause->getHint();
+
+    // TODO: Fix the type when everything about typing is final.
+    llvm::Value *HintInst =
+        (!Hint)
+            ? nullptr
+            : Builder.CreateIntCast(EmitScalarExpr(Hint), CGM.Int32Ty, false);
+
+    auto FiniCB = [this](InsertPointTy IP) {
+      CGBuilderTy::InsertPointGuard IPG(Builder);
+      assert(IP.getBlock()->end() != IP.getPoint() &&
+             "OpenMP IR Builder should cause terminated block!");
+      llvm::BasicBlock *IPBB = IP.getBlock();
+      llvm::BranchInst *IPBBTI =
+          llvm::dyn_cast<llvm::BranchInst>(IPBB->getTerminator());
+      llvm::BasicBlock *DestBB = IPBBTI->getSuccessor(0);
+
+      // erase and replace with cleanup branch.
+      IPBB->getTerminator()->eraseFromParent();
+      Builder.SetInsertPoint(IPBB);
+      CodeGenFunction::JumpDest Dest = getJumpDestInCurrentScope(DestBB);
+      EmitBranchThroughCleanup(Dest);
+    };
+
+    auto BodyGenCB = [CriticalRegionBodyStmt, this](InsertPointTy AllocaIP,
+                                                    InsertPointTy CodeGenIP,
+                                                    llvm::BasicBlock &FiniBB) {
+      auto OldAllocaIP = AllocaInsertPt;
+      if (AllocaIP.isSet())
+        AllocaInsertPt = &*AllocaIP.getPoint();
+      auto OldReturnBlock = ReturnBlock;
+      ReturnBlock = getJumpDestInCurrentScope(&FiniBB);
+
+      llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
+      if (llvm::Instruction *CodeGenIPBBTI = CodeGenIPBB->getTerminator())
+        CodeGenIPBBTI->eraseFromParent();
+
+      Builder.SetInsertPoint(CodeGenIPBB);
+
+      EmitStmt(CriticalRegionBodyStmt);
+
+      Builder.CreateBr(&FiniBB);
+
+      AllocaInsertPt = OldAllocaIP;
+      ReturnBlock = OldReturnBlock;
+    };
+
+    Builder.restoreIP(OMPBuilder->CreateCritical(
+        Builder, BodyGenCB, FiniCB, S.getDirectiveName().getAsString(),
+        HintInst));
+
+    return;
+  }
+
   auto &&CodeGen = [&S](CodeGenFunction &CGF, PrePostActionTy &Action) {
     Action.Enter(CGF);
     CGF.EmitStmt(S.getInnermostCapturedStmt()->getCapturedStmt());
   };
   const Expr *Hint = nullptr;
   if (const auto *HintClause = S.getSingleClause<OMPHintClause>())
     Hint = HintClause->getHint();
   OMPLexicalScope Scope(*this, S, OMPD_unknown);

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

 #define LLVM_OPENMP_CONSTANTS_H
 
 #include "llvm/ADT/BitmaskEnum.h"
 #include "llvm/ADT/StringRef.h"
 
 namespace llvm {
 class Type;
 class Module;
+class ArrayType;
 class StructType;
 class PointerType;
 class FunctionType;
 
 namespace omp {
 LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
 
 /// IDs for all OpenMP directives.

 /// Forward declarations for LLVM-IR types (simple, function and structure) are
 /// generated below. Their names are defined and used in OpenMP/OMPKinds.def.
 /// Here we provide the forward declarations, the initializeTypes function will
 /// provide the values.
 ///
 ///{
 namespace types {
 
+/// Type for kmp_critical_name[8], and related pointer type;
+// extern ArrayType *KmpCriticalNameTy;
+// extern PointerType *KmpCriticalNamePtrTy;
+
 #define OMP_TYPE(VarName, InitValue) extern Type *VarName;
+#define OMP_ARRAY_TYPE(VarName, ElemTy, ArraySize)                             \
+  extern ArrayType *VarName##Ty;                                               \
+  extern PointerType *VarName##PtrTy;
 #define OMP_FUNCTION_TYPE(VarName, IsVarArg, ReturnType, ...)                  \
   extern FunctionType *VarName;                                                \
   extern PointerType *VarName##Ptr;
 #define OMP_STRUCT_TYPE(VarName, StrName, ...)                                 \
   extern StructType *VarName;                                                  \
   extern PointerType *VarName##Ptr;
 #include "llvm/Frontend/OpenMP/OMPKinds.def"
 

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

private:
   /// The LLVM-IR Builder used to create IR.
   IRBuilder<> Builder;
 
   /// Map to remember source location strings
   StringMap<Constant *> SrcLocStrMap;
 
   /// Map to remember existing ident_t*.
   DenseMap<std::pair<Constant *, uint64_t>, GlobalVariable *> IdentMap;
+
+  /// An ordered map of auto-generated variables to their unique names.
+  /// It stores variables with the following names: 1) ".gomp_critical_user_" +
+  /// <critical_section_name> + ".var" for "omp critical" directives; 2)
+  /// <mangled_name_for_global_var> + ".cache." for cache for threadprivate
+  /// variables.
+  StringMap<AssertingVH<Constant>, BumpPtrAllocator> InternalVars;
+
+public:
+  /// Generator for '#omp master'
+  ///
+  /// \param Loc The insert and source location description.
+  /// \param BodyGenCB Callback that will generate the region code.
+  ///
+  /// \returns The insertion position *after* the master.
+  InsertPointTy CreateMaster(const LocationDescription &Loc,
+                             BodyGenCallbackTy BodyGenCB,
+                             FinalizeCallbackTy FiniCB);
+
+  /// Generator for '#omp master'
+  ///
+  /// \param Loc The insert and source location description.
+  /// \param BodyGenCB Callback that will generate the region code.
+  /// \param CriticalName name of the lock used by the critical directive
+  /// \param hasHint whether there is ahint clause associated with critical
+  /// \param FiniCB Callback to finalize variable copies.
+  ///
+  /// \returns The insertion position *after* the master.
+  InsertPointTy CreateCritical(const LocationDescription &Loc,
+                               BodyGenCallbackTy BodyGenCB,
+                               FinalizeCallbackTy FiniCB,
+                               StringRef CriticalName, Value *HintInst);
+
+private:
+  /// Common interface for generating entry calls for OMP Directives.
+  /// if the directive has a region/body, It will set the insertion
+  /// point to the body
+  ///
+  /// \param OMPD Directive to generate entry blocks for
+  /// \param EntryCall Call to the entry OMP Runtime Function
+  /// \param ExitBB block where the region ends.
+  /// \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.
+  ///
+  /// \return The insertion position in exit block
+  InsertPointTy emitCommonDirectiveEntry(omp::Directive OMPD,
+                                         llvm::Value *EntryCall,
+                                         BasicBlock *ExitBB,
+                                         bool conditional = false);
+
+  /// Common interface to finalize the region
+  ///
+  /// \param OMPD Directive to generate exiting code for
+  /// \param IP Insertion point for emitting Finalization code and exit call
+  /// \param ExitBB Exit BasicBlock for OMP region
+  /// \param ExitCall Call to the ending OMP Runtime Function
+  /// \param hasFinalize indicate if the directive will require finalization
+  ///        and has a finalization callback in the stack that
+  /// should be
+  ///        called.
+  ///
+  /// \return The insertion position in exit block
+  InsertPointTy emitCommonDirectiveExit(omp::Directive OMPD, InsertPointTy IP,
+                                        BasicBlock *ExitBB,
+                                        Instruction *ExitCall,
+                                        bool hasFinalize);
+
+  /// Common Interface to generate OMP inlined regions
+  ///
+  /// \param OMPD Directive to generate inlined region for
+  /// \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.
+  ///
+  /// \return The insertion point after the region
+
+  InsertPointTy EmitOMPInlinedRegion(omp::Directive OMPD,
+                                     Instruction *EntryCall,
+                                     Instruction *ExitCall,
+                                     BodyGenCallbackTy BodyGenCB,
+                                     FinalizeCallbackTy FiniCB,
+                                     bool conditional, bool hasFinalize);
+
+  std::string getName(ArrayRef<StringRef> Parts, StringRef FirstSeparator,
+                      StringRef Separator) const;
+
+  Constant *getOrCreateOMPInternalVariable(llvm::Type *Ty,
+                                           const llvm::Twine &Name,
+                                           unsigned AddressSpace = 0);
+
+  Value *getOMPCriticalRegionLock(StringRef CriticalName);
+
+  llvm::Constant *getOrCreateInternalVariable(llvm::Type *Ty,
+                                              const llvm::Twine &Name,
+                                              unsigned AddressSpace);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_IR_IRBUILDER_H

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def

 __OMP_TYPE(Int8Ptr)
 __OMP_TYPE(Int32Ptr)
 
 #undef __OMP_TYPE
 #undef OMP_TYPE
 
 ///}
 
+/// array types
+///
+///{
+
+#ifndef OMP_ARRAY_TYPE
+#define OMP_ARRAY_TYPE(VarName, ElemTy, ArraySize)
+#endif
+
+#define __OMP_ARRAY_TYPE(VarName, ElemTy, ArraySize)               \
+  OMP_ARRAY_TYPE(VarName, ElemTy, ArraySize)
+
+__OMP_ARRAY_TYPE(KmpCriticalName, Int32, 8)
+
+#undef __OMP_ARRAY_TYPE
+#undef OMP_ARRAY_TYPE
+
+///}
+
 /// Struct and function types
 ///
 ///{
 
 #ifndef OMP_STRUCT_TYPE
 #define OMP_STRUCT_TYPE(VarName, StructName, ...)
 #endif
 

 __OMP_RTL(__kmpc_push_num_threads, false, Void, IdentPtr, Int32,
           /* Int */ Int32)
 __OMP_RTL(__kmpc_push_proc_bind, false, Void, IdentPtr, Int32, /* Int */ Int32)
 __OMP_RTL(__kmpc_serialized_parallel, false, Void, IdentPtr, Int32)
 __OMP_RTL(__kmpc_end_serialized_parallel, false, Void, IdentPtr, Int32)
 
 __OMP_RTL(omp_get_thread_num, false, Int32, )
 
+__OMP_RTL(__kmpc_omp_master, false, Int32, IdentPtr, Int32)
+__OMP_RTL(__kmpc_omp_end_master, false, Void, IdentPtr, Int32)
+__OMP_RTL(__kmpc_omp_critical, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy)
+__OMP_RTL(__kmpc_omp_critical_with_hint, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy, Int32)
+__OMP_RTL(__kmpc_omp_end_critical, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy)
+
 #undef __OMP_RTL
 #undef OMP_RTL
 
 #define EnumAttr(Kind) Attribute::get(Ctx, Attribute::AttrKind::Kind)
 #define AttributeSet(...)                                                      \
   AttributeSet::get(Ctx, ArrayRef<Attribute>({__VA_ARGS__}))
 
 #ifndef OMP_ATTRS_SET

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

 }
 
 /// Declarations for LLVM-IR types (simple, function and structure) are
 /// generated below. Their names are defined and used in OpenMPKinds.def. Here
 /// we provide the declarations, the initializeTypes function will provide the
 /// values.
 ///
 ///{
-
+// ArrayType *llvm::omp::types::KmpCriticalNameTy = nullptr;
+// PointerType *llvm::omp::types::KmpCriticalNamePtrTy = nullptr;
 #define OMP_TYPE(VarName, InitValue) Type *llvm::omp::types::VarName = nullptr;
+#define OMP_ARRAY_TYPE(VarName, ElemTy, ArraySize)                             \
+  ArrayType *llvm::omp::types::VarName##Ty = nullptr;                          \
+  PointerType *llvm::omp::types::VarName##PtrTy = nullptr;
 #define OMP_FUNCTION_TYPE(VarName, IsVarArg, ReturnType, ...)                  \
   FunctionType *llvm::omp::types::VarName = nullptr;                           \
   PointerType *llvm::omp::types::VarName##Ptr = nullptr;
 #define OMP_STRUCT_TYPE(VarName, StrName, ...)                                 \
   StructType *llvm::omp::types::VarName = nullptr;                             \
   PointerType *llvm::omp::types::VarName##Ptr = nullptr;
 #include "llvm/Frontend/OpenMP/OMPKinds.def"
 
 ///}
 
 void llvm::omp::types::initializeTypes(Module &M) {
   if (Void)
     return;
 
   LLVMContext &Ctx = M.getContext();
+  //  KmpCriticalNameTy = ArrayType::get(
+  //            Type::getInt32Ty(M.getContext()),/*NumElements*/ 8);
+  //  KmpCriticalNamePtrTy = PointerType::getUnqual(KmpCriticalNameTy);
   // Create all simple and struct types exposed by the runtime and remember
   // the llvm::PointerTypes of them for easy access later.
   StructType *T;
 #define OMP_TYPE(VarName, InitValue) VarName = InitValue;
+#define OMP_ARRAY_TYPE(VarName, ElemTy, ArraySize)                             \
+  VarName##Ty = ArrayType::get(ElemTy, ArraySize);                             \
+  VarName##PtrTy = PointerType::getUnqual(VarName##Ty);
 #define OMP_FUNCTION_TYPE(VarName, IsVarArg, ReturnType, ...)                  \
   VarName = FunctionType::get(ReturnType, {__VA_ARGS__}, IsVarArg);            \
   VarName##Ptr = PointerType::getUnqual(VarName);
 #define OMP_STRUCT_TYPE(VarName, StructName, ...)                              \
   T = M.getTypeByName(StructName);                                             \
   if (!T)                                                                      \
     T = StructType::create(Ctx, {__VA_ARGS__}, StructName);                    \
   VarName = T;                                                                 \

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

IRBuilder<>::InsertPoint OpenMPIRBuilder::CreateParallel(
   InsertPointTy PreFiniIP(PRegPreFiniBB, PreFiniTI->getIterator());
   FiniCB(PreFiniIP);
 
   for (Instruction *I : ToBeDeleted)
     I->eraseFromParent();
 
   return AfterIP;
 }
+
+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 = getOrCreateRuntimeFunction(OMPRTL___kmpc_omp_master);
+  Instruction *EntryCall = Builder.CreateCall(EntryRTLFn, Args);
+
+  Function *ExitRTLFn =
+      getOrCreateRuntimeFunction(OMPRTL___kmpc_omp_end_master);
+  Instruction *ExitCall = Builder.CreateCall(ExitRTLFn, Args);
+
+  return EmitOMPInlinedRegion(OMPD, EntryCall, ExitCall, BodyGenCB, FiniCB,
+                              /*Conditional*/ true, /*hasFinalize*/ true);
+}
+
+OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::CreateCritical(
+    const LocationDescription &Loc, BodyGenCallbackTy BodyGenCB,
+    FinalizeCallbackTy FiniCB, StringRef CriticalName, Value *HintInst) {
+
+  if (!updateToLocation(Loc))
+    return Loc.IP;
+
+  Directive OMPD = Directive::OMPD_critical;
+  Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
+  Value *Ident = getOrCreateIdent(SrcLocStr);
+  Value *ThreadId = getOrCreateThreadID(Ident);
+  Value *LockVar = getOMPCriticalRegionLock(CriticalName);
+  llvm::Value *Args[] = {Ident, ThreadId, LockVar};
+
+  SmallVector<llvm::Value *, 4> EnterArgs(std::begin(Args), std::end(Args));
+  Function *fn = nullptr;
+  if (HintInst) {
+    // Add Hint to entry Args and create call
+    EnterArgs.push_back(HintInst);
+    fn = getOrCreateRuntimeFunction(OMPRTL___kmpc_omp_critical_with_hint);
+  } else {
+    fn = getOrCreateRuntimeFunction(OMPRTL___kmpc_omp_critical);
+  }
+  Instruction *EntryCall = Builder.CreateCall(fn, EnterArgs);
+
+  Function *ExitRTLFn =
+      getOrCreateRuntimeFunction(OMPRTL___kmpc_omp_end_critical);
+  Instruction *ExitCall = Builder.CreateCall(ExitRTLFn, Args);
+
+  return EmitOMPInlinedRegion(OMPD, EntryCall, ExitCall, BodyGenCB, FiniCB,
+                              /*Conditional*/ false, /*hasFinalize*/ true);
+}
+
+OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::EmitOMPInlinedRegion(
+    omp::Directive OMPD, Instruction *EntryCall, Instruction *ExitCall,
+    BodyGenCallbackTy BodyGenCB, FinalizeCallbackTy FiniCB, bool conditional,
+    bool hasFinalize) {
+
+  FinalizationStack.push_back({FiniCB, OMPD, /*IsCancellable*/ false});
+
+  // Create 'Critical' 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");
+  BasicBlock *FiniBB =
+      EntryBB->splitBasicBlock(EntryBB->getTerminator(), "omp_region.finalize");
+
+  Builder.SetInsertPoint(EntryBB->getTerminator());
+  emitCommonDirectiveEntry(OMPD, EntryCall, ExitBB, conditional);
+
+  // generate body
+  BodyGenCB(/* AllocaIP */ InsertPointTy(),
+            /* CodeGenIP */ Builder.saveIP(), *FiniBB);
+
+  // emit exit call and do any needed finalization.
+  auto FinIP = InsertPointTy(FiniBB, FiniBB->getFirstInsertionPt());
+  assert(FiniBB->getTerminator()->getNumSuccessors() == 1 &&
+         FiniBB->getTerminator()->getSuccessor(0) == ExitBB &&
+         "Unexpected insertion point for finalization call!");
+  emitCommonDirectiveExit(OMPD, FinIP, ExitBB, ExitCall, /*hasFinalize*/ true);
+
+  BasicBlock *IPBB = SplitPos->getParent();
+  assert(IPBB == ExitBB && "Unexpected Insertion point location!");
+
+  if (!isa_and_nonnull<BranchInst>(SplitPos)) {
+    SplitPos->eraseFromParent();
+  }
+
+  Builder.SetInsertPoint(ExitBB);
+
+  return Builder.saveIP();
+}
+
+OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::emitCommonDirectiveEntry(
+    omp::Directive omp, llvm::Value *EntryCall, BasicBlock *ExitBB,
+    bool conditional) {
+
+  OpenMPIRBuilder::InsertPointTy Contpt;
+  llvm::BasicBlock *EntryBB = Builder.GetInsertBlock();
+
+  if (conditional) {
+    llvm::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
+    // body generation next. Place the block after the current block.
+    llvm::Function *CurFn = EntryBB->getParent();
+    CurFn->getBasicBlockList().insertAfter(EntryBB->getIterator(), ThenBB);
+
+    // Move Entry branch to end of ThenBB, and replace with conditional
+    // branch (If-stmt)
+    Instruction *EntryBBTI = EntryBB->getTerminator();
+    Builder.CreateCondBr(CallBool, ThenBB, ExitBB);
+    EntryBBTI->removeFromParent();
+    Builder.SetInsertPoint(UI);
+    Builder.Insert(EntryBBTI);
+    UI->eraseFromParent();
+    Builder.SetInsertPoint(ThenBB->getTerminator());
+
+    // return an insertion point to ExitBB.
+    Contpt = IRBuilder<>::InsertPoint(ExitBB, ExitBB->getFirstInsertionPt());
+  } else
+    // otherwise Return an insertion point to current block
+    Contpt = Builder.saveIP();
+
+  return Contpt;
+}
+
+OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::emitCommonDirectiveExit(
+    omp::Directive OMPD, InsertPointTy IP, BasicBlock *ExitBB,
+    Instruction *ExitCall, bool hasFinalize) {
+
+  IRBuilder<>::InsertPointGuard IPG(Builder);
+  Builder.restoreIP(IP);
+
+  // If there is finalization to do, emit it before the exit call
+  if (hasFinalize) {
+    assert(!FinalizationStack.empty() &&
+           "Unexpected finalization stack state!");
+
+    FinalizationInfo Fi = FinalizationStack.pop_back_val();
+    assert(Fi.DK == OMPD && "Unexpected Directive for Finalization call!");
+
+    Fi.FiniCB(IP);
+
+    BasicBlock *InsertBB = IP.getBlock();
+    Instruction *InsertBBTI = InsertBB->getTerminator();
+    if (!(InsertBBTI)) {
+      Builder.SetInsertPoint(InsertBB);
+      InsertBBTI = Builder.CreateBr(ExitBB);
+    }
+
+    // set Builder IP for call creation
+    Builder.SetInsertPoint(InsertBBTI);
+  }
+
+  ExitCall->removeFromParent();
+  Builder.Insert(ExitCall);
+
+  return IRBuilder<>::InsertPoint(ExitCall->getParent(),
+                                  ExitCall->getIterator());
+}
+
+std::string OpenMPIRBuilder::getName(ArrayRef<StringRef> Parts,
+                                     StringRef FirstSeparator,
+                                     StringRef Separator) const {
+  SmallString<128> Buffer;
+  llvm::raw_svector_ostream OS(Buffer);
+  StringRef Sep = FirstSeparator;
+  for (StringRef Part : Parts) {
+    OS << Sep << Part;
+    Sep = Separator;
+  }
+  return OS.str();
+}
+
+Constant *OpenMPIRBuilder::getOrCreateOMPInternalVariable(
+    llvm::Type *Ty, const llvm::Twine &Name, unsigned AddressSpace) {
+  SmallString<256> Buffer;
+  llvm::raw_svector_ostream Out(Buffer);
+  Out << Name;
+  StringRef RuntimeName = Out.str();
+  auto &Elem = *InternalVars.try_emplace(RuntimeName, nullptr).first;
+  if (Elem.second) {
+    assert(Elem.second->getType()->getPointerElementType() == Ty &&
+           "OMP internal variable has different type than requested");
+    return &*Elem.second;
+  }
+
+  return Elem.second = new llvm::GlobalVariable(
+             M, Ty, /*IsConstant*/ false, llvm::GlobalValue::CommonLinkage,
+             llvm::Constant::getNullValue(Ty), Elem.first(),
+             /*InsertBefore=*/nullptr, llvm::GlobalValue::NotThreadLocal,
+             AddressSpace);
+}
+
+Value *OpenMPIRBuilder::getOMPCriticalRegionLock(StringRef CriticalName) {
+  std::string Prefix = Twine("gomp_critical_user_", CriticalName).str();
+  std::string Name = getName({Prefix, "var"}, ".", ".");
+  return getOrCreateOMPInternalVariable(KmpCriticalNameTy, Name);
+}

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

if (ExitBB)
       ASSERT_EQ(CI->getNextNode()->getSuccessor(0), ExitBB);
     else
       ExitBB = CI->getNextNode()->getSuccessor(0);
     ASSERT_EQ(ExitBB->size(), 1U);
     ASSERT_TRUE(isa<ReturnInst>(ExitBB->front()));
   }
 }
 
+TEST_F(OpenMPIRBuilderTest, MasterDirective) {
+  using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+  F->setName("func");
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+
+  AllocaInst *PrivAI = nullptr;
+
+  BasicBlock *EntryBB = nullptr;
+  BasicBlock *FinalBB = nullptr;
+  BasicBlock *ExitBB = nullptr;
+  BasicBlock *ThenBB = nullptr;
+
+  auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+                       BasicBlock &FiniBB) {
+    if (AllocaIP.isSet())
+      Builder.restoreIP(AllocaIP);
+    else
+      Builder.SetInsertPoint(&*(F->getEntryBlock().getFirstInsertionPt()));
+    PrivAI = Builder.CreateAlloca(F->arg_begin()->getType());
+    Builder.CreateStore(F->arg_begin(), PrivAI);
+
+    llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
+    llvm::Instruction *CodeGenIPInst = &*CodeGenIP.getPoint();
+    EXPECT_EQ(CodeGenIPBB->getTerminator(), CodeGenIPInst);
+
+    Builder.restoreIP(CodeGenIP);
+
+    // collect some info for checks later
+    FinalBB = &FiniBB;
+    ExitBB = FiniBB.getUniqueSuccessor();
+    ThenBB = Builder.GetInsertBlock();
+    EntryBB = ThenBB->getUniquePredecessor();
+
+    // simple instructions for body
+    Value *PrivLoad = Builder.CreateLoad(PrivAI, "local.use");
+    Builder.CreateICmpNE(F->arg_begin(), PrivLoad);
+  };
+
+  auto FiniCB = [&](InsertPointTy IP) {
+    BasicBlock *IPBB = IP.getBlock();
+    EXPECT_NE(IPBB->end(), IP.getPoint());
+  };
+
+  Builder.restoreIP(OMPBuilder.CreateMaster(Builder, BodyGenCB, FiniCB));
+  Value *EntryBBTI = EntryBB->getTerminator();
+  EXPECT_NE(EntryBBTI, nullptr);
+  EXPECT_TRUE(isa<BranchInst>(EntryBBTI));
+  BranchInst *EntryBr = cast<BranchInst>(EntryBB->getTerminator());
+  EXPECT_TRUE(EntryBr->isConditional());
+  EXPECT_EQ(EntryBr->getSuccessor(0), ThenBB);
+  EXPECT_EQ(ThenBB->getUniqueSuccessor(), FinalBB);
+  EXPECT_EQ(FinalBB->getUniqueSuccessor(), ExitBB);
+  EXPECT_EQ(EntryBr->getSuccessor(1), ExitBB);
+
+  CmpInst *CondInst = cast<CmpInst>(EntryBr->getCondition());
+  EXPECT_TRUE(isa<CallInst>(CondInst->getOperand(0)));
+
+  CallInst *MasterEntryCI = cast<CallInst>(CondInst->getOperand(0));
+  EXPECT_EQ(MasterEntryCI->getNumArgOperands(), 2U);
+  EXPECT_EQ(MasterEntryCI->getCalledFunction()->getName(), "__kmpc_omp_master");
+  EXPECT_TRUE(isa<GlobalVariable>(MasterEntryCI->getArgOperand(0)));
+
+  CallInst *MasterEndCI = nullptr;
+  for (auto &FI : *FinalBB) {
+    Instruction *cur = &FI;
+    if (isa<CallInst>(cur)) {
+      MasterEndCI = cast<CallInst>(cur);
+      if (MasterEndCI->getCalledFunction()->getName() ==
+          "__kmpc_omp_end_master")
+        break;
+      else
+        MasterEndCI = nullptr;
+    }
+  }
+  EXPECT_NE(MasterEndCI, nullptr);
+  EXPECT_EQ(MasterEndCI->getNumArgOperands(), 2U);
+  EXPECT_TRUE(isa<GlobalVariable>(MasterEndCI->getArgOperand(0)));
+  EXPECT_EQ(MasterEndCI->getArgOperand(1), MasterEntryCI->getArgOperand(1));
+}
+
+TEST_F(OpenMPIRBuilderTest, CriticalDirective) {
+  using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+  F->setName("func");
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+
+  AllocaInst *PrivAI = Builder.CreateAlloca(F->arg_begin()->getType());
+
+  BasicBlock *EntryBB = nullptr;
+  BasicBlock *FinalBB = nullptr;
+  BasicBlock *ExitBB = nullptr;
+
+  auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+                       BasicBlock &FiniBB) {
+    // collect some info for checks later
+    FinalBB = &FiniBB;
+    ExitBB = FiniBB.getUniqueSuccessor();
+    EntryBB = FinalBB->getUniquePredecessor();
+
+    // actual start for bodyCB
+    llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
+    llvm::Instruction *CodeGenIPInst = &*CodeGenIP.getPoint();
+    EXPECT_EQ(CodeGenIPBB->getTerminator(), CodeGenIPInst);
+    EXPECT_EQ(EntryBB, CodeGenIPBB);
+
+    // body begin
+    Builder.restoreIP(CodeGenIP);
+    Builder.CreateStore(F->arg_begin(), PrivAI);
+    Value *PrivLoad = Builder.CreateLoad(PrivAI, "local.use");
+    Builder.CreateICmpNE(F->arg_begin(), PrivLoad);
+  };
+
+  auto FiniCB = [&](InsertPointTy IP) {
+    BasicBlock *IPBB = IP.getBlock();
+    EXPECT_NE(IPBB->end(), IP.getPoint());
+  };
+
+  Builder.restoreIP(OMPBuilder.CreateCritical(Builder, BodyGenCB, FiniCB,
+                                              "testCRT", nullptr));
+
+  Value *EntryBBTI = EntryBB->getTerminator();
+  EXPECT_NE(EntryBBTI, nullptr);
+  EXPECT_TRUE(isa<BranchInst>(EntryBBTI));
+  BranchInst *EntryBr = cast<BranchInst>(EntryBB->getTerminator());
+  EXPECT_FALSE(EntryBr->isConditional());
+  EXPECT_EQ(EntryBB->getUniqueSuccessor(), FinalBB);
+  EXPECT_EQ(FinalBB->getUniqueSuccessor(), ExitBB);
+
+  CallInst *CriticalEntryCI = nullptr;
+  for (auto &EI : *EntryBB) {
+    Instruction *cur = &EI;
+    if (isa<CallInst>(cur)) {
+      CriticalEntryCI = cast<CallInst>(cur);
+      if (CriticalEntryCI->getCalledFunction()->getName() ==
+          "__kmpc_omp_critical")
+        break;
+      else
+        CriticalEntryCI = nullptr;
+    }
+  }
+  EXPECT_NE(CriticalEntryCI, nullptr);
+  EXPECT_EQ(CriticalEntryCI->getNumArgOperands(), 3U);
+  EXPECT_EQ(CriticalEntryCI->getCalledFunction()->getName(),
+            "__kmpc_omp_critical");
+  EXPECT_TRUE(isa<GlobalVariable>(CriticalEntryCI->getArgOperand(0)));
+
+  CallInst *CriticalEndCI = nullptr;
+  for (auto &FI : *FinalBB) {
+    Instruction *cur = &FI;
+    if (isa<CallInst>(cur)) {
+      CriticalEndCI = cast<CallInst>(cur);
+      if (CriticalEndCI->getCalledFunction()->getName() ==
+          "__kmpc_omp_end_critical")
+        break;
+      else
+        CriticalEndCI = nullptr;
+    }
+  }
+  EXPECT_NE(CriticalEndCI, nullptr);
+  EXPECT_EQ(CriticalEndCI->getNumArgOperands(), 3U);
+  EXPECT_TRUE(isa<GlobalVariable>(CriticalEndCI->getArgOperand(0)));
+  EXPECT_EQ(CriticalEndCI->getArgOperand(1), CriticalEntryCI->getArgOperand(1));
+  PointerType *CriticalNamePtrTy =
+      PointerType::getUnqual(ArrayType::get(Type::getInt32Ty(Ctx), 8));
+  EXPECT_EQ(CriticalEndCI->getArgOperand(2), CriticalEntryCI->getArgOperand(2));
+  EXPECT_EQ(CriticalEndCI->getArgOperand(2)->getType(), CriticalNamePtrTy);
+}
+
 } // namespace