[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim closed this revision.
fghanim added a comment.

commited:  rG82b8236cf248 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks a lot! Sorry for the delay in reviewing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim added a comment.

Ping. 
Does this patch need further changes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim added a comment.

you are responding to a comment from 2 weeks ago, so let's just move on.

I uploaded a new patch yesterday. You have any comments on this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added subscribers: raghavendhra, jhuber6, anchu-rajendran, kiranktp, 
DavidTruby, ronlieb, kiranchandramohan.
jdoerfert added a comment.

In D79675#2058657 , @fghanim wrote:

> I am moving on because we are not getting anywhere. However, There are few 
> things I need to point out very quickly.
>
> > I fail to see the point in committing for example your target type solution 
> > if we found a more generic version in the meantime.
> >  We can for sure commit them and then replace them subsequently, but is 
> > that really helping anyone? It would not be a question if
> >  they were in, since they are not it seems to me there is no benefit in 
> > blocking the other patch on them. I mean, the time you worked
> >  on that part is not "less wasted" if we commit it. TBH, I don't thin it is 
> > wasted at all but that is a different conversation.
>
> At one point, you said I was delaying D80222 
>  moments after it was uploaded. Now, D79675 
>  and D79676 
>  , cannot be committed because of the 
> artificial dependency on that patch.


That is not the reason these patches cannot be committed. The reason is that 
*parts of them* needed changes and another round of review.

>> I'm sorry you **feel** I waste your time. I really would not do so on 
>> purpose.
> 
> It is not a feeling. It is a matter of record, and never said you did so on 
> purpose. Freudian slip? :p

Wouldn't that logic imply that any code replacement causes the original work to 
be "wasted time"? I assume that is not the case, hence I do not assume you 
wasted yours (wrt. the code).

>> While more reviewers would obviously help, it is known that smaller patches 
>> do too.
> 
> D79739  has been merged with D80222 
> . I kinda feel bad for the reviewer ;)
>  You are the code owner of the `OMPBuilder`, who do you suggest as reviewers 
> that I can add, in the future?

As of now, I don't really see anyone else doing reviews. I was hoping you would 
start reviewing patches. Same for @jhuber6 
at some point. Other likely candidates are: @kiranchandramohan, @DavidTruby 
@kiranktp @anchu-rajendran @raghavendhra @ronlieb

>> If you have ideas on other improvements of the process, I'm happy to try 
>> them out.
> 
> Let people know that you changed your mind before they put in the time and 
> effort. I am sure that is not a big ask.

I believe I did let everyone know as early as I knew. I'm unsure how I should 
improve on this.
I mention my thoughts in reviews and I usually include a ping to relevant 
@people.
I also assume (or hope) that interested parties watch phabricator (or the 
mailing list), e.g., for OpenMP patches, so they stay informed about what is 
happening.

>  ---
> 
> Anyways, I suggested something that you didn't reply to, which you may have 
> missed. To resolve this, would you be willing to go for:
> 
> 1. You handle any typing problems with this patch when you commit it and 
> D79676  after D80222 
> 
> 2. I bring back all my runtime def.s that I need, and use macros per your 
> original suggestion, and you commit this and D79676 
>  today or tomorrow and that patch can merge 
> based on head commit which it will do anyway?

I'm not even sure I follow. D80222  landed, as 
far as I can tell. Can you elaborate on these suggestions so I do not 
misinterpret them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim updated this revision to Diff 269583.
fghanim added a comment.
Herald added a subscriber: aaron.ballman.

- Rebase + refactor based on D80222 
- addressed reviewer comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675

Files:
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -779,4 +779,41 @@
   EXPECT_EQ(CriticalEndCI->getArgOperand(2)->getType(), CriticalNamePtrTy);
 }
 
+TEST_F(OpenMPIRBuilderTest, CopyinBlocks) {
+  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());
+  IntegerType* Int32 = Type::getInt32Ty(M->getContext());
+  AllocaInst* MasterAddress = Builder.CreateAlloca(Int32->getPointerTo());
+	AllocaInst* PrivAddress = Builder.CreateAlloca(Int32->getPointerTo());
+
+  BasicBlock *EntryBB = BB;
+
+  OMPBuilder.CreateCopyinClauseBlocks(Builder.saveIP(), MasterAddress, PrivAddress, Int32, /*BranchtoEnd*/true);
+
+  BranchInst* EntryBr = dyn_cast_or_null(EntryBB->getTerminator());
+
+  EXPECT_NE(EntryBr, nullptr);
+  EXPECT_TRUE(EntryBr->isConditional());
+
+  BasicBlock* NotMasterBB = EntryBr->getSuccessor(0);
+  BasicBlock* CopyinEnd = EntryBr->getSuccessor(1);
+  CmpInst* CMP = dyn_cast_or_null(EntryBr->getCondition());
+
+  EXPECT_NE(CMP, nullptr);
+  EXPECT_NE(NotMasterBB, nullptr);
+  EXPECT_NE(CopyinEnd, nullptr);
+
+  BranchInst* NotMasterBr = dyn_cast_or_null(NotMasterBB->getTerminator());
+  EXPECT_NE(NotMasterBr, nullptr);
+  EXPECT_FALSE(NotMasterBr->isConditional());
+  EXPECT_EQ(CopyinEnd,NotMasterBr->getSuccessor(0));
+}
+
 } // namespace
Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -611,9 +611,6 @@
  "Unexpected finalization stack state!");
 
   Instruction *PRegPreFiniTI = PRegPreFiniBB->getTerminator();
-  assert(PRegPreFiniTI->getNumSuccessors() == 1 &&
- PRegPreFiniTI->getSuccessor(0) == PRegExitBB &&
- "Unexpected CFG structure!");
 
   InsertPointTy PreFiniIP(PRegPreFiniBB, PRegPreFiniTI->getIterator());
   FiniCB(PreFiniIP);
@@ -948,6 +945,105 @@
   ExitCall->getIterator());
 }
 
+OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::CreateCopyinClauseBlocks(
+InsertPointTy IP, Value *MasterAddr, Value *PrivateAddr,
+llvm::IntegerType *IntPtrTy, bool BranchtoEnd) {
+  if (!IP.isSet())
+return IP;
+
+  IRBuilder<>::InsertPointGuard IPG(Builder);
+
+  // creates the following CFG structure
+  //	   OMP_Entry : (MasterAddr != PrivateAddr)?
+  //   F T
+  //   |  \
+  //   | copin.not.master
+  //   |  /
+  //   v /
+  //   copyin.not.master.end
+  //		 |
+  // v
+  //   OMP.Entry.Next
+
+  BasicBlock *OMP_Entry = IP.getBlock();
+  Function *CurFn = OMP_Entry->getParent();
+  BasicBlock *CopyBegin =
+  BasicBlock::Create(M.getContext(), "copyin.not.master", CurFn);
+  BasicBlock *CopyEnd = nullptr;
+
+  // If entry block is terminated, split to preserve the branch to following
+  // basic block (i.e. OMP.Entry.Next), otherwise, leave everything as is.
+  if (isa_and_nonnull(OMP_Entry->getTerminator())) {
+CopyEnd = OMP_Entry->splitBasicBlock(OMP_Entry->getTerminator(),
+ "copyin.not.master.end");
+OMP_Entry->getTerminator()->eraseFromParent();
+  } else {
+CopyEnd =
+BasicBlock::Create(M.getContext(), "copyin.not.master.end", CurFn);
+  }
+
+  Builder.SetInsertPoint(OMP_Entry);
+  Value *MasterPtr = Builder.CreatePtrToInt(MasterAddr, IntPtrTy);
+  Value *PrivatePtr = Builder.CreatePtrToInt(PrivateAddr, IntPtrTy);
+  Value *cmp = Builder.CreateICmpNE(MasterPtr, PrivatePtr);
+  Builder.CreateCondBr(cmp, CopyBegin, CopyEnd);
+
+  Builder.SetInsertPoint(CopyBegin);
+  if (BranchtoEnd)
+Builder.SetInsertPoint(Builder.CreateBr(CopyEnd));
+
+  return Builder.saveIP();
+}
+
+CallInst *OpenMPIRBuilder::CreateOMPAlloc(const LocationDescription ,
+  Value *Size, Value *Allocator,
+  std::string Name) {
+  IRBuilder<>::InsertPointGuard IPG(Builder);
+  Builder.restoreIP(Loc.IP);
+
+  Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
+  Value *Ident = 

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim marked an inline comment as done.
fghanim added a comment.

I am moving on because we are not getting anywhere. However, There are few 
things I need to point out very quickly.

> I fail to see the point in committing for example your target type solution 
> if we found a more generic version in the meantime.
>  We can for sure commit them and then replace them subsequently, but is that 
> really helping anyone? It would not be a question if
>  they were in, since they are not it seems to me there is no benefit in 
> blocking the other patch on them. I mean, the time you worked
>  on that part is not "less wasted" if we commit it. TBH, I don't thin it is 
> wasted at all but that is a different conversation.

At one point, you said I was delaying D80222  
moments after it was uploaded. Now, D79675  
and D79676  , cannot be committed because of 
the artificial dependency on that patch.

> I'm sorry you **feel** I waste your time. I really would not do so on purpose.

It is not a feeling. It is a matter of record, and never said you did so on 
purpose. Freudian slip? :p

> While more reviewers would obviously help, it is known that smaller patches 
> do too.

D79739  has been merged with D80222 
. I kinda feel bad for the reviewer ;)
You are the code owner of the `OMPBuilder`, who do you suggest as reviewers 
that I can add, in the future?

> If you have ideas on other improvements of the process, I'm happy to try them 
> out.

Let people know that you changed your mind before they put in the time and 
effort. I am sure that is not a big ask.

---

Anyways, I suggested something that you didn't reply to, which you may have 
missed. To resolve this, would you be willing to go for:

1. You handle any typing problems with this patch when you commit it and D79676 
 after D80222 
2. I bring back all my runtime def.s that I need, and use macros per your 
original suggestion, and you commit this and D79676 
 today or tomorrow and that patch can merge 
based on head commit which it will do anyway?




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:62
+  /// return a copy of the current insertion point information
+  InsertPointTy SaveIP() { return Builder.saveIP(); }
+

jdoerfert wrote:
> fghanim wrote:
> > jdoerfert wrote:
> > > Unused?
> > I'll happily drop them if you want. I needed them at one point, and assumed 
> > we may need them later, and left them in to see what you think. So still 
> > LGTM , or no LGTM?
> Generally we should not include code we don't need (or that is not tested).
We don't need it at the moment, however, I do not think an IRBuilder should go 
without a way to specify where you want it to point at.
This doesn't need a test. it just passes an argument to a private `IRBuilder` - 
if that works, this should just work.
Bottom line, should I remove it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D79675#2051682 , @fghanim wrote:

> > My goal is to save us time, during development, review, maintenance, and 
> > future extensions. I hope you know that.
>
> I am certain of that. However, I am starting to have doubts if my time is 
> accounted for as part of "save us time".


That is unfortunate because it is. Even if you don't believe me, it seems 
illogical for me to waste your time on purpose assuming I benefit from the 
work, wouldn't you agree?

Since I can neither change what I've said before, nor predict how future 
changes impact my past comments, I will not argue with you on me changing my 
mind. 
That happens even without outside interference during a code review.

In this particular situation I suggested something and someone came up with 
something better in a different patch, I will not defend something for the sake 
of having suggested
it in the first place. Instead, I made you aware of it and asked if there is a 
reason to not adopt the better solution we haven't considered before. This is 
the same as with code.
We replace code from anyone with something better as it becomes available. That 
is a good thing, not something to argue about.

I fail to see the point in committing for example your target type solution if 
we found a more generic version in the meantime.
We can for sure commit them and then replace them subsequently, but is that 
really helping anyone? It would not be a question if
they were in, since they are not it seems to me there is no benefit in blocking 
the other patch on them. I mean, the time you worked
on that part is not "less wasted" if we commit it. TBH, I don't thin it is 
wasted at all but that is a different conversation.

---

I'm sorry you feel I waste your time. I really would not do so on purpose. In 
order to avoid such situations we should have quick reviews.
I already try to provide feedback as soon as I can. While more reviewers would 
obviously help, it is known that smaller patches do too.
If you have ideas on other improvements of the process, I'm happy to try them 
out.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:62
+  /// return a copy of the current insertion point information
+  InsertPointTy SaveIP() { return Builder.saveIP(); }
+

fghanim wrote:
> jdoerfert wrote:
> > Unused?
> I'll happily drop them if you want. I needed them at one point, and assumed 
> we may need them later, and left them in to see what you think. So still LGTM 
> , or no LGTM?
Generally we should not include code we don't need (or that is not tested).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim marked 2 inline comments as done.
fghanim added a comment.

I am going to omit parts of the quote, because who wants to look at a wall of 
test - readability is important ;)

In D79675#2051079 , @jdoerfert wrote:

> In D79675#2047154 , @fghanim wrote:
>
> > Until the OMPBuilder becomes the way to CG OMP IR, we will always be 
> > playing catch-up. All the OMPKinds def.s are very copy/paste-able one or 
> > two-liners and very easy to move. However, the actual code to CG the IR is 
> > not.
>
>
> That is not true, with the other patch we require all new code gen 
> functionality to list the runtime functions in OMPKinds.def.


That is not what I said, and FWIW I don't disagree.

> This is not about number of lines. Don't take my word for it, ask the 
> community if you want. We want the smallest logical and testable patches 
> possible, unrelated to the size. (FWIW, 300 lines is not nothing either.)

Number of lines has some correlation with readability, and readability is a 
factor.

> The time spend arguing is more than splitting would have taken in the first 
> place.

In my defense, I have long build times as a result of frequent updates. ;)
As for this one, I am doing it out of work hours :p

> Conceptual parts:
> 
> - Target dependent types (which we can initialize w/o the frontend based on 
> the datalayout)
> - Insert point changes (which seem to be unsued in this patch)
> - The create functions

This is the current state of the patch, not what it was before it was gutted by 
various updates. Back then it was a "the smallest logical and testable patches 
possible, unrelated to the size". it was `createXXX` methods along with all 
related types and run time calls. FWIW, it was modeled after D70109 
 , which -I can only assume- fit yours and the 
community's criteria? ;)

> With D80222  we don't need the first. If you 
> think the way it's done in there is for some reason less good, please say so, 
> otherwise I fail to see why we would not go ahead with that one and rebase 
> this on top.

I'll suggest something below. I want to be done with this patch.

>> However, What wasted everyone's time my friend, is removing integral parts 
>> to this patch which has two other feature patches depend on it, which meant 
>> I needed to build and rebuild to make sure things still work. it would have 
>> been way easier to make D79739  modify and 
>> build on the typing in this one as I suggested there, and in retrospect, is 
>> something I should've pushed harder for. Anyways, let's move on. :D
> 
> I don't think this is true, even if, this is not how this works. The only way 
> to make fast progress is small patches. I give you almost instant feedback on 
> you patches but the more is included in one, the more revision we have to go 
> through. Unrelated problems stall parts we depend on.
> 
> Maybe your setup needs tweaking or you should bundle changes in smaller patch 
> from the beginning to avoid this, either way, the guidelines are clear:
>  https://llvm.org/docs/CodeReview.html

The only reason I brought my setup and long build times up is to indicate to 
you to please be mindful of my time with your comments. per the guidelines: 
"Aim to Make Efficient Use of **Everyone’s** Time" - emphasis is mine

Let me recap what happened for your benefit and to see if the guidelines were 
followed: (all of this can be verified - it is a matter of record, not an 
opinion)

- I upload the patch on 9th.
- On the 11th, You ask me to make the `void*` and macro changes.
- Also on the 11th, patch D79739  was created 
and included many of the things I had, and was using `ptr8*` in place of 
`void*` - i.e. **had no definition for `void*`**. You also ask the author of 
that patch to make the same macro changes. You don't tell me someone else is 
working on it or anything.
- On the 12th, I respond to some items and start working on implementing the 
things I didn't comment on as part of new patch update. - I am still unaware 
others are working on things that were asked of me
- On the 13th, you told me about patch D79739  
, and that they implemented the `void*` macros you asked me to do - at which 
point I had them implemented and verified, but not uploaded
- Also, on the 13th, you tell the author of that patch, that the 
**target-typing issues will be handled by me**. I ask you if I should drop the 
duplicate def.s between this and the other patch. and when you didn't respond 
to that, I drop them because it makes more sense to have these things as part 
of D79739 
- On the 14th, I uploaded a patch with these changes, and I keep 
target-specific types and Def.s that weren't in that patch - we exchange 
comments, and you ask me to change 

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D79675#2047154 , @fghanim wrote:

> In D79675#2045826 , @jdoerfert wrote:
>
> > In D79675#2045563 , @fghanim wrote:
> >
> > > So this whole thing was about moving Def.s out of `CGOMPRuntime`? Given 
> > > how low priority making the-soon-to-be-deprecated `CGOMPRuntime` use the 
> > > new Def.s is, and that I actually use these stuff as part of 
> > > the-soon-to-be-the-way-to-CG-OMP `OMPBuilder`, wouldn't it have been 
> > > better to postpone both patches until we are done with this one then add 
> > > anything I didn't have already as part of D79739 
> > >  ? It would have certainly saved 
> > > everyone a lot of time, and made more sense given that the earlier patch 
> > > came out 2 days after mine, and the other patch today? :)
> >
> >
> >
> >
> > 1. Soon is relative.
>
>
> "Soon" is indeed relative, and so is "later", and so is 99% of the words. 
> However, words have specific meanings, otherwise opposites would refer to the 
> same thing, and words become useless and meaningless. "Soon" means soon.
>
> > 2. In the meantime people work on clang and add runtime functions into the 
> > CGOMPRuntime but not into OMPKinds.def. We are playing catch up all the 
> > time, thus wasting time.
>
> Until the `OMPBuilder` becomes the way to CG OMP IR, we will always be 
> playing catch-up. All the `OMPKinds` def.s are very copy/paste-able one or 
> two-liners and very easy to move. However, the actual code to CG the IR is 
> not.


That is not true, with the other patch we require all new code gen 
functionality to list the runtime functions in OMPKinds.def.

> Therefore, I hereby declare, that henceforth, for as long as I am working on 
> the `OMPBuilder`, if people are willing to write the CG code of new things as 
> part of the `OMPBuilder`, I am happy to be the one to move the def.s for them 
> at anytime, including on my deathbed. :D
> 
>> 3. I said it before, please split them up in small pieces. It does really 
>> not help if we combine unrelated things in a single patch. It doesn't make 
>> it faster and it is not less work at the end of the day.
> 
> Johannes Comon .. This patch IS Small (~300 lines) and everything here is 
> related (with one exception below).

This is not about number of lines. Don't take my word for it, ask the community 
if you want. We want the smallest logical and testable patches possible, 
unrelated to the size. (FWIW, 300 lines is not nothing either.)

>   This patch adds 4 new  `create` calls to the `OMPBuilder` needed by the 
> privatization stuff in patches D79676 and D79677 . These calls require 
> certain runtime calls and typing def.s. It is how we have always done it for 
> any new `create` method starting with `CreateOMPParallel()`. The Def.s I 
> added are almost completely gone. The exception I mentioned earlier is the 
> pass-throughs to the `OMPBuilder`'s `IRBuilder` which were LGTM-ed early on 
> and nothing uses them yet, so it is really a non-issue.

The time spend arguing is more than splitting would have taken in the first 
place.

Conceptual parts:

- Target dependent types (which we can initialize w/o the frontend based on the 
datalayout)
- Insert point changes (which seem to be unsued in this patch)
- The create functions

With D80222  we don't need the first. If you 
think the way it's done in there is for some reason less good, please say so, 
otherwise I fail to see why we would not go ahead with that one and rebase this 
on top.
The insert point changes could probably go away, or be part of a change that 
actually replaces the `Builder.` uses with them.

> However, What wasted everyone's time my friend, is removing integral parts to 
> this patch which has two other feature patches depend on it, which meant I 
> needed to build and rebuild to make sure things still work. it would have 
> been way easier to make D79739  modify and 
> build on the typing in this one as I suggested there, and in retrospect, is 
> something I should've pushed harder for. Anyways, let's move on. :D

I don't think this is true, even if, this is not how this works. The only way 
to make fast progress is small patches. I give you almost instant feedback on 
you patches but the more is included in one, the more revision we have to go 
through. Unrelated problems stall parts we depend on.

Maybe your setup needs tweaking or you should bundle changes in smaller patch 
from the beginning to avoid this, either way, the guidelines are clear:
https://llvm.org/docs/CodeReview.html



My goal is to save us time, during development, review, maintenance, and future 
extensions. I hope you know that.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:62
+  /// return a copy of the current 

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim updated this revision to Diff 265293.
fghanim added a comment.

Addressing more reviewers comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPConstants.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/lib/Transforms/IPO/OpenMPOpt.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -62,7 +62,7 @@
 
 TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(M->getDataLayout().getIntPtrType(M->getContext()));
 
   IRBuilder<> Builder(BB);
 
@@ -102,7 +102,7 @@
 TEST_F(OpenMPIRBuilderTest, CreateCancel) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(M->getDataLayout().getIntPtrType(M->getContext()));
 
   BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
   new UnreachableInst(Ctx, CBB);
@@ -157,7 +157,7 @@
 TEST_F(OpenMPIRBuilderTest, CreateCancelIfCond) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(M->getDataLayout().getIntPtrType(M->getContext()));
 
   BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
   new UnreachableInst(Ctx, CBB);
@@ -218,7 +218,7 @@
 TEST_F(OpenMPIRBuilderTest, CreateCancelBarrier) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(M->getDataLayout().getIntPtrType(M->getContext()));
 
   BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
   new UnreachableInst(Ctx, CBB);
@@ -272,7 +272,7 @@
 
 TEST_F(OpenMPIRBuilderTest, DbgLoc) {
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(M->getDataLayout().getIntPtrType(M->getContext()));
   F->setName("func");
 
   IRBuilder<> Builder(BB);
@@ -308,7 +308,7 @@
 TEST_F(OpenMPIRBuilderTest, ParallelSimple) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(M->getDataLayout().getIntPtrType(M->getContext()));
   F->setName("func");
   IRBuilder<> Builder(BB);
 
@@ -405,7 +405,7 @@
 TEST_F(OpenMPIRBuilderTest, ParallelIfCond) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(M->getDataLayout().getIntPtrType(M->getContext()));
   F->setName("func");
   IRBuilder<> Builder(BB);
 
@@ -516,7 +516,7 @@
 TEST_F(OpenMPIRBuilderTest, ParallelCancelBarrier) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(M->getDataLayout().getIntPtrType(M->getContext()));
   F->setName("func");
   IRBuilder<> Builder(BB);
 
@@ -625,7 +625,7 @@
 TEST_F(OpenMPIRBuilderTest, MasterDirective) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(M->getDataLayout().getIntPtrType(M->getContext()));
   F->setName("func");
   IRBuilder<> Builder(BB);
 
@@ -704,7 +704,7 @@
 TEST_F(OpenMPIRBuilderTest, CriticalDirective) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(M->getDataLayout().getIntPtrType(M->getContext()));
   F->setName("func");
   IRBuilder<> Builder(BB);
 
@@ -779,4 +779,41 @@
   EXPECT_EQ(CriticalEndCI->getArgOperand(2)->getType(), CriticalNamePtrTy);
 }
 
+TEST_F(OpenMPIRBuilderTest, CopyinBlocks) {
+  using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize(M->getDataLayout().getIntPtrType(M->getContext()));
+  F->setName("func");
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+
+  AllocaInst *PrivAI = Builder.CreateAlloca(F->arg_begin()->getType());
+  IntegerType* Int32 = Type::getInt32Ty(M->getContext());
+  AllocaInst* MasterAddress = Builder.CreateAlloca(Int32->getPointerTo());
+	AllocaInst* PrivAddress = Builder.CreateAlloca(Int32->getPointerTo());
+
+  BasicBlock *EntryBB = BB;
+
+  OMPBuilder.CreateCopyinClauseBlocks(Builder.saveIP(), MasterAddress, PrivAddress, Int32, /*BranchtoEnd*/true);
+
+  BranchInst* EntryBr = dyn_cast_or_null(EntryBB->getTerminator());
+
+  EXPECT_NE(EntryBr, nullptr);
+  EXPECT_TRUE(EntryBr->isConditional());
+
+  BasicBlock* 

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim marked an inline comment as done.
fghanim added inline comments.



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:86
+  llvm::omp::types::Int8PtrPtr = Int8Ptr->getPointerTo();
+  llvm::omp::types::Int8PtrPtrPtr = Int8PtrPtr->getPointerTo();
+

fghanim wrote:
> jdoerfert wrote:
> > I think the macro way to specify pointer is easier to use.
> It is. But that patch has landed yet, and so I cannot use that. so for the 
> time being, I am going to keep this way. and after both patches land, I'll 
> make a minor patch that will just make this small modification.
I meant to say the patch has *not* landed yet. sorry for the confusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim marked 10 inline comments as done.
fghanim added a comment.

In D79675#2045826 , @jdoerfert wrote:

> In D79675#2045563 , @fghanim wrote:
>
> > So this whole thing was about moving Def.s out of `CGOMPRuntime`? Given how 
> > low priority making the-soon-to-be-deprecated `CGOMPRuntime` use the new 
> > Def.s is, and that I actually use these stuff as part of 
> > the-soon-to-be-the-way-to-CG-OMP `OMPBuilder`, wouldn't it have been better 
> > to postpone both patches until we are done with this one then add anything 
> > I didn't have already as part of D79739  ? 
> > It would have certainly saved everyone a lot of time, and made more sense 
> > given that the earlier patch came out 2 days after mine, and the other 
> > patch today? :)
>
>
>
>
> 1. Soon is relative.


"Soon" is indeed relative, and so is "later", and so is 99% of the words. 
However, words have specific meanings, otherwise opposites would refer to the 
same thing, and words become useless and meaningless. "Soon" means soon.

> 2. In the meantime people work on clang and add runtime functions into the 
> CGOMPRuntime but not into OMPKinds.def. We are playing catch up all the time, 
> thus wasting time.

Until the `OMPBuilder` becomes the way to CG OMP IR, we will always be playing 
catch-up. All the `OMPKinds` def.s are very copy/paste-able one or two-liners 
and very easy to move. However, the actual code to CG the IR is not. 
Therefore, I hereby declare, that henceforth, for as long as I am working on 
the `OMPBuilder`, if people are willing to write the CG code of new things as 
part of the `OMPBuilder`, I am happy to be the one to move the def.s for them 
at anytime, including on my deathbed. :D

> 3. I said it before, please split them up in small pieces. It does really not 
> help if we combine unrelated things in a single patch. It doesn't make it 
> faster and it is not less work at the end of the day.

Johannes Comon .. This patch IS Small (~300 lines) and everything here is 
related (with one exception below). This patch adds 4 new  `create` calls 
to the `OMPBuilder` needed by the privatization stuff in patches D79676 
 and D79677  
. These calls require certain runtime calls and typing def.s. It is how we have 
always done it for any new `create` method starting with 
`CreateOMPParallel()`. The Def.s I added are almost completely gone. The 
exception I mentioned earlier is the pass-throughs to the `OMPBuilder`'s 
`IRBuilder` which were LGTM-ed early on and nothing uses them yet, so it is 
really a non-issue.

However, What wasted everyone's time my friend, is removing integral parts to 
this patch which has two other feature patches depend on it, which meant I 
needed to build and rebuild to make sure things still work. it would have been 
way easier to make D79739  modify and build on 
the typing in this one as I suggested there, and in retrospect, is something I 
should've pushed harder for. Anyways, let's move on. :D

Let me know, if there are any further comments.




Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:86
+  llvm::omp::types::Int8PtrPtr = Int8Ptr->getPointerTo();
+  llvm::omp::types::Int8PtrPtrPtr = Int8PtrPtr->getPointerTo();
+

jdoerfert wrote:
> I think the macro way to specify pointer is easier to use.
It is. But that patch has landed yet, and so I cannot use that. so for the time 
being, I am going to keep this way. and after both patches land, I'll make a 
minor patch that will just make this small modification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D79675#2045563 , @fghanim wrote:

> In D79675#2045405 , @jdoerfert wrote:
>
> > > Could you please list the other patches that are being held back by this 
> > > one? I'd be interested to have a look at them. :)
> >
> > We need the target type support for D80222 
> > , D79739  
> > can go in but we need to modify it afterwards.
>
>
> So this whole thing was about moving Def.s out of `CGOMPRuntime`? Given how 
> low priority making the-soon-to-be-deprecated `CGOMPRuntime` use the new 
> Def.s is, and that I actually use these stuff as part of 
> the-soon-to-be-the-way-to-CG-OMP `OMPBuilder`, wouldn't it have been better 
> to postpone both patches until we are done with this one then add anything I 
> didn't have already as part of D79739  ? It 
> would have certainly saved everyone a lot of time, and made more sense given 
> that the earlier patch came out 2 days after mine, and the other patch today? 
> :)




1. Soon is relative.
2. In the meantime people work on clang and add runtime functions into the 
CGOMPRuntime but not into OMPKinds.def. We are playing catch up all the time, 
thus wasting time.
3. I said it before, please split them up in small pieces. It does really not 
help if we combine unrelated things in a single patch. It doesn't make it 
faster and it is not less work at the end of the day.

> In any case, I addressed everything based on your earlier comments. Thanks 
> for reviewing my patches. Let me know if you think other changes are needed 
> here, otherwise could you please commit this for me, I still don't have 
> commit access.

Thanks. Comments added.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:62
+
+  /// Set Insert point to Instruction /p IPII
+  void setInsertPoint(Instruction *IPII) { Builder.SetInsertPoint(IPII); }

Nit: \p not /p (Or does both work?)



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:86
+  llvm::omp::types::Int8PtrPtr = Int8Ptr->getPointerTo();
+  llvm::omp::types::Int8PtrPtrPtr = Int8PtrPtr->getPointerTo();
+

I think the macro way to specify pointer is easier to use.



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:109
+  llvm::omp::types::SizeTy = (SizeTy) ? SizeTy : Int32;
+}
+

I guess we don't need anymore.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:104
+  initializeSizeTy(SizeTy);
+}
 

Probably won't need these either.



Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:61
 initializeRuntimeFunctions();
-OMPBuilder.initialize();
+OMPBuilder.initialize(Type::getInt32Ty(M.getContext()));
   }

Is size_t always 32 bit? I think you can use the datalayout from the module via 
`getIntPtrType`.



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:65
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(Type::getInt32Ty(M->getContext()));
 

Same as above, also below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim added a comment.

In D79675#2045405 , @jdoerfert wrote:

> > Could you please list the other patches that are being held back by this 
> > one? I'd be interested to have a look at them. :)
>
> We need the target type support for D80222 , 
> D79739  can go in but we need to modify it 
> afterwards.


So this whole thing was about moving Def.s out of `CGOMPRuntime`? Given how low 
priority making the-soon-to-be-deprecated `CGOMPRuntime` use the new Def.s is, 
and that I actually use these stuff as part of the-soon-to-be-the-way-to-CG-OMP 
`OMPBuilder`, wouldn't it have been better to postpone both patches until we 
are done with this one then add anything I didn't have already as part of 
D79739  ? It would have certainly saved 
everyone a lot of time, and made more sense given that the earlier patch came 
out 2 days after mine, and the other patch today? :)

In any case, I addressed everything based on your earlier comments. Thanks for 
reviewing my patches. Let me know if you think other changes are needed here, 
otherwise could you please commit this for me, I still don't have commit access.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

> Could you please list the other patches that are being held back by this one? 
> I'd be interested to have a look at them. :)

We need the target type support for D80222 , 
D79739  can go in but we need to modify it 
afterwards.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim updated this revision to Diff 265071.
fghanim marked 2 inline comments as done.
fghanim added a comment.

Addressing reviewer Comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPConstants.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/lib/Transforms/IPO/OpenMPOpt.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -62,7 +62,7 @@
 
 TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(Type::getInt32Ty(M->getContext()));
 
   IRBuilder<> Builder(BB);
 
@@ -102,7 +102,7 @@
 TEST_F(OpenMPIRBuilderTest, CreateCancel) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(Type::getInt32Ty(M->getContext()));
 
   BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
   new UnreachableInst(Ctx, CBB);
@@ -157,7 +157,7 @@
 TEST_F(OpenMPIRBuilderTest, CreateCancelIfCond) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(Type::getInt32Ty(M->getContext()));
 
   BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
   new UnreachableInst(Ctx, CBB);
@@ -218,7 +218,7 @@
 TEST_F(OpenMPIRBuilderTest, CreateCancelBarrier) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(Type::getInt32Ty(M->getContext()));
 
   BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
   new UnreachableInst(Ctx, CBB);
@@ -272,7 +272,7 @@
 
 TEST_F(OpenMPIRBuilderTest, DbgLoc) {
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(Type::getInt32Ty(M->getContext()));
   F->setName("func");
 
   IRBuilder<> Builder(BB);
@@ -308,7 +308,7 @@
 TEST_F(OpenMPIRBuilderTest, ParallelSimple) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(Type::getInt32Ty(M->getContext()));
   F->setName("func");
   IRBuilder<> Builder(BB);
 
@@ -405,7 +405,7 @@
 TEST_F(OpenMPIRBuilderTest, ParallelIfCond) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(Type::getInt32Ty(M->getContext()));
   F->setName("func");
   IRBuilder<> Builder(BB);
 
@@ -516,7 +516,7 @@
 TEST_F(OpenMPIRBuilderTest, ParallelCancelBarrier) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(Type::getInt32Ty(M->getContext()));
   F->setName("func");
   IRBuilder<> Builder(BB);
 
@@ -625,7 +625,7 @@
 TEST_F(OpenMPIRBuilderTest, MasterDirective) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(Type::getInt32Ty(M->getContext()));
   F->setName("func");
   IRBuilder<> Builder(BB);
 
@@ -704,7 +704,7 @@
 TEST_F(OpenMPIRBuilderTest, CriticalDirective) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(Type::getInt32Ty(M->getContext()));
   F->setName("func");
   IRBuilder<> Builder(BB);
 
@@ -779,4 +779,41 @@
   EXPECT_EQ(CriticalEndCI->getArgOperand(2)->getType(), CriticalNamePtrTy);
 }
 
+TEST_F(OpenMPIRBuilderTest, CopyinBlocks) {
+  using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize(Type::getInt32Ty(M->getContext()));
+  F->setName("func");
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+
+  AllocaInst *PrivAI = Builder.CreateAlloca(F->arg_begin()->getType());
+  IntegerType* Int32 = Type::getInt32Ty(M->getContext());
+  AllocaInst* MasterAddress = Builder.CreateAlloca(Int32->getPointerTo());
+	AllocaInst* PrivAddress = Builder.CreateAlloca(Int32->getPointerTo());
+
+  BasicBlock *EntryBB = BB;
+
+  OMPBuilder.CreateCopyinClauseBlocks(Builder.saveIP(), MasterAddress, PrivAddress, Int32, /*BranchtoEnd*/true);
+
+  BranchInst* EntryBr = dyn_cast_or_null(EntryBB->getTerminator());
+
+  EXPECT_NE(EntryBr, nullptr);
+  EXPECT_TRUE(EntryBr->isConditional());
+
+  BasicBlock* NotMasterBB = EntryBr->getSuccessor(0);
+  BasicBlock* CopyinEnd = EntryBr->getSuccessor(1);
+  CmpInst* CMP = 

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim marked 21 inline comments as done.
fghanim added a comment.

In D79675#2044809 , @jdoerfert wrote:

> What's the status? Can we split the target specific types stuff if this may 
> take a while, other patches depend on that :)


Could you please list the other patches that are being held back by this one? 
I'd be interested to have a look at them. :)




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:234
 __OMP_RTL(__kmpc_critical, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy)
-__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, Int32)
+__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, IntPtrTy)
 __OMP_RTL(__kmpc_end_critical, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy)

jdoerfert wrote:
> fghanim wrote:
> > jdoerfert wrote:
> > > fghanim wrote:
> > > > jdoerfert wrote:
> > > > > fghanim wrote:
> > > > > > jdoerfert wrote:
> > > > > > > I think this was correct before:
> > > > > > > ```
> > > > > > >   KMP_EXPORT void __kmpc_critical_with_hint(ident_t *, kmp_int32 
> > > > > > > global_tid, kmp_critical_name *, uint32_t hint);
> > > > > > > ```
> > > > > > Nop, it's supposed to be whatever `IntPtrTy` is in the frontend 
> > > > > > (i.e. 32 for 32bit, 64 for 64bit).
> > > > > > 
> > > > > > `IntPtrTy` is actually a union with `sizety` in `CodeGenModule`
> > > > > I'm confused. I copied the declaration above from the runtime. It 
> > > > > doesn't contain an `IntPtrTy` or similar. I agree that `IntPtrTy` is 
> > > > > machine dependent and we need your initializers. What I tried to say 
> > > > > is that at least one declaration in the runtime has 
> > > > > `__kmpc_critical_with_hint` with an int32 as last argument. That 
> > > > > said, the runtime is not shy of incompatible declarations for 
> > > > > functions.
> > > > I cannot speak for what's in the runtime. However, in clang, this 
> > > > currently is defined to use `IntPtrTy`. If you go check, I have a todo 
> > > > in the current implementation for critical to come back and fix it.
> > > That is just an existing bug in Clang. The runtime is consistent with the 
> > > type and arguably it is the runtime we must match, not the other way 
> > > around ;)
> > Apparently, this used to be `uintptr_t` in the runtime and was changed by 
> > D51235 . It doesn't say why the change was made.
> > 
> > Clang uses this in multiple places, which makes me think it may have been 
> > intentional. So I suggest we go by the standard. Where can I find what does 
> > the OMP standard say about how the signature of the runtime calls should 
> > look like? This way I'll fix this one accordingly, and later we may go and 
> > make sure that all the rest match up with the standard - where possible.
> This is not a standard function. It is a runtime function. I suspect it is a 
> int32_t because enums default to `int` which is 32 bit on all interesting 
> platforms. Should probably be a `int` but I would need to read up to confirm. 
> Long story short, and as I said before, it is a bug in clang. The code here 
> was correct.
> 
> FWIW, we will soon replace clang's runtime function generation with these 
> macros. That will happen in the next days I hope.
Oh, my bad. I thought OMP runtime functions are designed a specific way based 
on the OMP standard. If that's not the case, then it doesn't matter. I'll drop 
this change as well. There is a handful of hints anyway.



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122
+  }
+}
+

jdoerfert wrote:
> fghanim wrote:
> > jdoerfert wrote:
> > > fghanim wrote:
> > > > jdoerfert wrote:
> > > > > (I doubt we need the `initVoidPtr` function but if we do, the 
> > > > > condition looks wrong)
> > > > Forgot to refractor when I changed names.
> > > > 
> > > > Regarding `void*` , I am not sure how it is defined in fortran. That's 
> > > > why I made it possible to initialize it separately.
> > > I see. Let's cross that bridge once we get to it.
> > Modified it a bit and removed usage from initialization. If fortran people 
> > don't end using it, then we can remove it.
> Let's not add stuff that might be used.
I just looked this up. Apparently, Fortran doesn't seem to have the concept of 
void pointer natively. However, They have something called `Assumed types` , 
which was added for interoperability with C, which *probably* means it will 
follow whatever C does to handle void pointers.

I'll remove it now, and if they choose, people can add it later.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:106
+  initializeVoidPtrTy(VoidPtrTy);
+}
+

jdoerfert wrote:
> fghanim wrote:
> > jdoerfert wrote:
> > > I guess we can directly call the initialize functions, right?
> > > With an assert that SizeTy is set we can make sure users do it ;)
> > Sure, then again, I am just 

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.
Herald added a subscriber: sstefan1.

What's the status? Can we split the target specific types stuff if this may 
take a while, other patches depend on that :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D79675#2037484 , @fghanim wrote:

> In D79675#2037314 , @jdoerfert wrote:
>
> > I left some comments on the type stuff. The rest looks good.
> >  I think if you rebase the type stuff on D79739 
> >  (which I can merge) we should only need 
> > to expand `initializeTypes` to make this work as expected. WDYT?
>
>
> Currently, I implemented the changes relevant to me from that and made a 
> separate local commit prior to this one to make sure things work.
>  I build llvm locally, and so it take 6 - 8 hours, so when all patches are 
> accepted, I'll do a rebase, etc. in one go to make sure there are no problems.


What takes 6-8h? Are you sure you don't want to merge parts that went through 
review as soon as possible? I mean, at least the type parts would be helpful.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:234
 __OMP_RTL(__kmpc_critical, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy)
-__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, Int32)
+__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, IntPtrTy)
 __OMP_RTL(__kmpc_end_critical, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy)

fghanim wrote:
> jdoerfert wrote:
> > fghanim wrote:
> > > jdoerfert wrote:
> > > > fghanim wrote:
> > > > > jdoerfert wrote:
> > > > > > I think this was correct before:
> > > > > > ```
> > > > > >   KMP_EXPORT void __kmpc_critical_with_hint(ident_t *, kmp_int32 
> > > > > > global_tid, kmp_critical_name *, uint32_t hint);
> > > > > > ```
> > > > > Nop, it's supposed to be whatever `IntPtrTy` is in the frontend (i.e. 
> > > > > 32 for 32bit, 64 for 64bit).
> > > > > 
> > > > > `IntPtrTy` is actually a union with `sizety` in `CodeGenModule`
> > > > I'm confused. I copied the declaration above from the runtime. It 
> > > > doesn't contain an `IntPtrTy` or similar. I agree that `IntPtrTy` is 
> > > > machine dependent and we need your initializers. What I tried to say is 
> > > > that at least one declaration in the runtime has 
> > > > `__kmpc_critical_with_hint` with an int32 as last argument. That said, 
> > > > the runtime is not shy of incompatible declarations for functions.
> > > I cannot speak for what's in the runtime. However, in clang, this 
> > > currently is defined to use `IntPtrTy`. If you go check, I have a todo in 
> > > the current implementation for critical to come back and fix it.
> > That is just an existing bug in Clang. The runtime is consistent with the 
> > type and arguably it is the runtime we must match, not the other way around 
> > ;)
> Apparently, this used to be `uintptr_t` in the runtime and was changed by 
> D51235 . It doesn't say why the change was made.
> 
> Clang uses this in multiple places, which makes me think it may have been 
> intentional. So I suggest we go by the standard. Where can I find what does 
> the OMP standard say about how the signature of the runtime calls should look 
> like? This way I'll fix this one accordingly, and later we may go and make 
> sure that all the rest match up with the standard - where possible.
This is not a standard function. It is a runtime function. I suspect it is a 
int32_t because enums default to `int` which is 32 bit on all interesting 
platforms. Should probably be a `int` but I would need to read up to confirm. 
Long story short, and as I said before, it is a bug in clang. The code here was 
correct.

FWIW, we will soon replace clang's runtime function generation with these 
macros. That will happen in the next days I hope.



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122
+  }
+}
+

fghanim wrote:
> jdoerfert wrote:
> > fghanim wrote:
> > > jdoerfert wrote:
> > > > (I doubt we need the `initVoidPtr` function but if we do, the condition 
> > > > looks wrong)
> > > Forgot to refractor when I changed names.
> > > 
> > > Regarding `void*` , I am not sure how it is defined in fortran. That's 
> > > why I made it possible to initialize it separately.
> > I see. Let's cross that bridge once we get to it.
> Modified it a bit and removed usage from initialization. If fortran people 
> don't end using it, then we can remove it.
Let's not add stuff that might be used.



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:69
 
 void llvm::omp::types::initializeTypes(Module ) {
   if (Void)

fghanim wrote:
> jdoerfert wrote:
> > Maybe we can/should take the IntPtr and SizeTy here so there is only a 
> > single call necessary that initializes all types. I fear "default" values 
> > are problematic.
> > 
> > WDYT?
> you mean pass it as a second argument? Sure, I would love that. I mean it's 
> fine with me either way. I originally wanted to do that. but then thought 
> maybe it is slightly better to 

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim marked 3 inline comments as done.
fghanim added a comment.

In D79675#2037314 , @jdoerfert wrote:

> I left some comments on the type stuff. The rest looks good.
>  I think if you rebase the type stuff on D79739 
>  (which I can merge) we should only need to 
> expand `initializeTypes` to make this work as expected. WDYT?


Currently, I implemented the changes relevant to me from that and made a 
separate local commit prior to this one to make sure things work.
I build llvm locally, and so it take 6 - 8 hours, so when all patches are 
accepted, I'll do a rebase, etc. in one go to make sure there are no problems.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:234
 __OMP_RTL(__kmpc_critical, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy)
-__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, Int32)
+__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, IntPtrTy)
 __OMP_RTL(__kmpc_end_critical, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy)

jdoerfert wrote:
> fghanim wrote:
> > jdoerfert wrote:
> > > fghanim wrote:
> > > > jdoerfert wrote:
> > > > > I think this was correct before:
> > > > > ```
> > > > >   KMP_EXPORT void __kmpc_critical_with_hint(ident_t *, kmp_int32 
> > > > > global_tid, kmp_critical_name *, uint32_t hint);
> > > > > ```
> > > > Nop, it's supposed to be whatever `IntPtrTy` is in the frontend (i.e. 
> > > > 32 for 32bit, 64 for 64bit).
> > > > 
> > > > `IntPtrTy` is actually a union with `sizety` in `CodeGenModule`
> > > I'm confused. I copied the declaration above from the runtime. It doesn't 
> > > contain an `IntPtrTy` or similar. I agree that `IntPtrTy` is machine 
> > > dependent and we need your initializers. What I tried to say is that at 
> > > least one declaration in the runtime has `__kmpc_critical_with_hint` with 
> > > an int32 as last argument. That said, the runtime is not shy of 
> > > incompatible declarations for functions.
> > I cannot speak for what's in the runtime. However, in clang, this currently 
> > is defined to use `IntPtrTy`. If you go check, I have a todo in the current 
> > implementation for critical to come back and fix it.
> That is just an existing bug in Clang. The runtime is consistent with the 
> type and arguably it is the runtime we must match, not the other way around ;)
Apparently, this used to be `uintptr_t` in the runtime and was changed by 
D51235 . It doesn't say why the change was made.

Clang uses this in multiple places, which makes me think it may have been 
intentional. So I suggest we go by the standard. Where can I find what does the 
OMP standard say about how the signature of the runtime calls should look like? 
This way I'll fix this one accordingly, and later we may go and make sure that 
all the rest match up with the standard - where possible.



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:69
 
 void llvm::omp::types::initializeTypes(Module ) {
   if (Void)

jdoerfert wrote:
> Maybe we can/should take the IntPtr and SizeTy here so there is only a single 
> call necessary that initializes all types. I fear "default" values are 
> problematic.
> 
> WDYT?
you mean pass it as a second argument? Sure, I would love that. I mean it's 
fine with me either way. I originally wanted to do that. but then thought maybe 
it is slightly better to explicitly initialize target specific data types, to 
give initialize type a cleaner look ... maybe?!



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:106
+  initializeVoidPtrTy(VoidPtrTy);
+}
+

jdoerfert wrote:
> I guess we can directly call the initialize functions, right?
> With an assert that SizeTy is set we can make sure users do it ;)
Sure, then again, I am just following in the stylistic footsteps of the 
pioneers who started the OMPBuilder, who created an initialize for initialize ;)

On second thought, I think I will just create something like 
`initializeTargetSpecificTypes()` sort of thing, and be done with it. instead 
of having one for each type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

I left some comments on the type stuff. The rest looks good.
I think if you rebase the type stuff on D79739 
 (which I can merge) we should only need to 
expand `initializeTypes` to make this work as expected. WDYT?




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:336
 
-  /// Generator for '#omp master'
+  /// Generator for '#omp Critical'
   ///

Nit: unrelated (just commit it), use lower case to match OpenMP



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:387
+  CallInst *CreateOMPFree(const LocationDescription , Value *Addr,
+  Value *Allocator, std::string name = "");
+

Style: Use `Name` for the variable name.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:389
+
+  /// Create a runtime call for kmpc_free
+  ///

Nit: Copy and paste



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:400
+  llvm::ConstantInt *Size,
+  const llvm::Twine  = Twine(" "));
+

Do we really want a space as name here? I would understand `""` but a space 
seems odd to me.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:234
 __OMP_RTL(__kmpc_critical, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy)
-__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, Int32)
+__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, IntPtrTy)
 __OMP_RTL(__kmpc_end_critical, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy)

fghanim wrote:
> jdoerfert wrote:
> > fghanim wrote:
> > > jdoerfert wrote:
> > > > I think this was correct before:
> > > > ```
> > > >   KMP_EXPORT void __kmpc_critical_with_hint(ident_t *, kmp_int32 
> > > > global_tid, kmp_critical_name *, uint32_t hint);
> > > > ```
> > > Nop, it's supposed to be whatever `IntPtrTy` is in the frontend (i.e. 32 
> > > for 32bit, 64 for 64bit).
> > > 
> > > `IntPtrTy` is actually a union with `sizety` in `CodeGenModule`
> > I'm confused. I copied the declaration above from the runtime. It doesn't 
> > contain an `IntPtrTy` or similar. I agree that `IntPtrTy` is machine 
> > dependent and we need your initializers. What I tried to say is that at 
> > least one declaration in the runtime has `__kmpc_critical_with_hint` with 
> > an int32 as last argument. That said, the runtime is not shy of 
> > incompatible declarations for functions.
> I cannot speak for what's in the runtime. However, in clang, this currently 
> is defined to use `IntPtrTy`. If you go check, I have a todo in the current 
> implementation for critical to come back and fix it.
That is just an existing bug in Clang. The runtime is consistent with the type 
and arguably it is the runtime we must match, not the other way around ;)



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:69
 
 void llvm::omp::types::initializeTypes(Module ) {
   if (Void)

Maybe we can/should take the IntPtr and SizeTy here so there is only a single 
call necessary that initializes all types. I fear "default" values are 
problematic.

WDYT?



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:106
+  initializeVoidPtrTy(VoidPtrTy);
+}
+

I guess we can directly call the initialize functions, right?
With an assert that SizeTy is set we can make sure users do it ;)



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:942
+  // v
+  //   OMP.Entry.Next
+

Great! thanks!



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:971
+Builder.SetInsertPoint(br);
+  }
+

Nit: `Br` or just make it a one-liner w/o braces.



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:817
+  EXPECT_EQ(CopyinEnd,NotMasterBr->getSuccessor(0));
+}
+

Great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim updated this revision to Diff 264006.
fghanim marked an inline comment as done.
fghanim added a comment.

- removed many Definitions from OMPKinds.def due to being added in D79739 

- made changes based on reviewer comments
- added unit test for `CreateCopyinClauseBlocks()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/lib/Frontend/OpenMP/OMPConstants.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -779,4 +779,41 @@
   EXPECT_EQ(CriticalEndCI->getArgOperand(2)->getType(), CriticalNamePtrTy);
 }
 
+TEST_F(OpenMPIRBuilderTest, CopyinBlocks) {
+  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());
+  IntegerType* Int32 = Type::getInt32Ty(M->getContext());
+  AllocaInst* MasterAddress = Builder.CreateAlloca(Int32->getPointerTo());
+	AllocaInst* PrivAddress = Builder.CreateAlloca(Int32->getPointerTo());
+
+  BasicBlock *EntryBB = BB;
+
+  OMPBuilder.CreateCopyinClauseBlocks(Builder.saveIP(), MasterAddress, PrivAddress, Int32, /*BranchtoEnd*/true);
+
+  BranchInst* EntryBr = dyn_cast_or_null(EntryBB->getTerminator());
+
+  EXPECT_NE(EntryBr, nullptr);
+  EXPECT_TRUE(EntryBr->isConditional());
+
+  BasicBlock* NotMasterBB = EntryBr->getSuccessor(0);
+  BasicBlock* CopyinEnd = EntryBr->getSuccessor(1);
+  CmpInst* CMP = dyn_cast_or_null(EntryBr->getCondition());
+
+  EXPECT_NE(CMP, nullptr);
+  EXPECT_NE(NotMasterBB, nullptr);
+  EXPECT_NE(CopyinEnd, nullptr);
+
+  BranchInst* NotMasterBr = dyn_cast_or_null(NotMasterBB->getTerminator());
+  EXPECT_NE(NotMasterBr, nullptr);
+  EXPECT_FALSE(NotMasterBr->isConditional());
+  EXPECT_EQ(CopyinEnd,NotMasterBr->getSuccessor(0));
+}
+
 } // namespace
Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -93,6 +93,18 @@
 
 void OpenMPIRBuilder::initialize() { initializeTypes(M); }
 
+void OpenMPIRBuilder::setIntPtrTy(IntegerType *IntPtrTy) {
+  initializeIntPtrTy(IntPtrTy);
+}
+
+void OpenMPIRBuilder::setSizeTy(IntegerType *SizeTy) {
+  initializeSizeTy(SizeTy);
+}
+
+void OpenMPIRBuilder::setVoidPtrTy(PointerType *VoidPtrTy) {
+  initializeVoidPtrTy(VoidPtrTy);
+}
+
 void OpenMPIRBuilder::finalize() {
   for (OutlineInfo  : OutlineInfos) {
 assert(!OI.Blocks.empty() &&
@@ -576,9 +588,6 @@
  "Unexpected finalization stack state!");
 
   Instruction *PRegPreFiniTI = PRegPreFiniBB->getTerminator();
-  assert(PRegPreFiniTI->getNumSuccessors() == 1 &&
- PRegPreFiniTI->getSuccessor(0) == PRegExitBB &&
- "Unexpected CFG structure!");
 
   InsertPointTy PreFiniIP(PRegPreFiniBB, PRegPreFiniTI->getIterator());
   FiniCB(PreFiniIP);
@@ -912,6 +921,106 @@
   ExitCall->getIterator());
 }
 
+OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::CreateCopyinClauseBlocks(
+InsertPointTy IP, Value *MasterAddr, Value *PrivateAddr,
+llvm::IntegerType *IntPtrTy, bool BranchtoEnd) {
+  if (!IP.isSet())
+return IP;
+
+  IRBuilder<>::InsertPointGuard IPG(Builder);
+
+  // creates the following CFG structure
+  //	   OMP_Entry : (MasterAddr != PrivateAddr)?
+  //   F T
+  //   |  \
+  //   | copin.not.master
+  //   |  /
+  //   v /
+  //   copyin.not.master.end
+  //		 |
+  // v
+  //   OMP.Entry.Next
+
+  BasicBlock *OMP_Entry = IP.getBlock();
+  Function *CurFn = OMP_Entry->getParent();
+  BasicBlock *CopyBegin =
+  BasicBlock::Create(M.getContext(), "copyin.not.master", CurFn);
+  BasicBlock *CopyEnd = nullptr;
+
+  // If entry block is terminated, split to preserve the branch to following
+  // basic block (i.e. OMP.Entry.Next), otherwise, leave everything as is.
+  if (isa_and_nonnull(OMP_Entry->getTerminator())) {
+CopyEnd = OMP_Entry->splitBasicBlock(OMP_Entry->getTerminator(),
+ "copyin.not.master.end");
+OMP_Entry->getTerminator()->eraseFromParent();
+  } else {
+CopyEnd =
+BasicBlock::Create(M.getContext(), "copyin.not.master.end", CurFn);
+  }
+
+  Builder.SetInsertPoint(OMP_Entry);
+  

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim marked 7 inline comments as done.
fghanim added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:101
+extern Type *IntPtrTy;
+extern Type *SizeTy;
+

jdoerfert wrote:
> jdoerfert wrote:
> > I can totally see why we need `int` and `size_t` but I'm not sure about the 
> > others.
> > `void` is universally translated to `i8*` Adding a pointer to a type should 
> > be done in OMPKinds.def, something like:
> > ```
> > #define __OMP_PTR_TYPE(NAME, BASE) OMP_TYPE(NAME, Base->getPointerTo());
> > __OMP_PTR_TYPE(VoidPtr, Int8)
> > __OMP_PTR_TYPE(VoidPtrPtr, VoidPtr)
> > ```
> My proposed scheme for the pointers was integrated in D79739 and will be 
> commited shortly.
Kept a couple a Ineed in OMPConstants. will add them in OMPKinds.def after the 
other patch lands.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:234
 __OMP_RTL(__kmpc_critical, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy)
-__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, Int32)
+__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, IntPtrTy)
 __OMP_RTL(__kmpc_end_critical, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy)

jdoerfert wrote:
> fghanim wrote:
> > jdoerfert wrote:
> > > I think this was correct before:
> > > ```
> > >   KMP_EXPORT void __kmpc_critical_with_hint(ident_t *, kmp_int32 
> > > global_tid, kmp_critical_name *, uint32_t hint);
> > > ```
> > Nop, it's supposed to be whatever `IntPtrTy` is in the frontend (i.e. 32 
> > for 32bit, 64 for 64bit).
> > 
> > `IntPtrTy` is actually a union with `sizety` in `CodeGenModule`
> I'm confused. I copied the declaration above from the runtime. It doesn't 
> contain an `IntPtrTy` or similar. I agree that `IntPtrTy` is machine 
> dependent and we need your initializers. What I tried to say is that at least 
> one declaration in the runtime has `__kmpc_critical_with_hint` with an int32 
> as last argument. That said, the runtime is not shy of incompatible 
> declarations for functions.
I cannot speak for what's in the runtime. However, in clang, this currently is 
defined to use `IntPtrTy`. If you go check, I have a todo in the current 
implementation for critical to come back and fix it.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:240
+__OMP_RTL(__kmpc_threadprivate_cached, false, VoidPtr, IdentPtr, Int32, 
VoidPtr, SizeTy, Void3Ptr)
+__OMP_RTL(__kmpc_threadprivate_register, false, Void, IdentPtr, VoidPtr, 
KmpcCtorTy, KmpcCopyCtorTy, KmpcDtorTy)
+

jdoerfert wrote:
> Can you add attributes (below) for these and call them in 
> `llvm/test/Transforms/OpenMP/add_attributes.ll` [if not already present]. 
> These changes and the additional types should go in a separate patch.
Added in D79739



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122
+  }
+}
+

jdoerfert wrote:
> fghanim wrote:
> > jdoerfert wrote:
> > > (I doubt we need the `initVoidPtr` function but if we do, the condition 
> > > looks wrong)
> > Forgot to refractor when I changed names.
> > 
> > Regarding `void*` , I am not sure how it is defined in fortran. That's why 
> > I made it possible to initialize it separately.
> I see. Let's cross that bridge once we get to it.
Modified it a bit and removed usage from initialization. If fortran people 
don't end using it, then we can remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D79675#2032764 , @fghanim wrote:

> This is a small patch as it is. splitting it any further would make it very 
> very small :D


Very small is great, then I can accept it fast, finding enough time to read 
larger patches is part of why it takes me so long to do reviews.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:101
+extern Type *IntPtrTy;
+extern Type *SizeTy;
+

jdoerfert wrote:
> I can totally see why we need `int` and `size_t` but I'm not sure about the 
> others.
> `void` is universally translated to `i8*` Adding a pointer to a type should 
> be done in OMPKinds.def, something like:
> ```
> #define __OMP_PTR_TYPE(NAME, BASE) OMP_TYPE(NAME, Base->getPointerTo());
> __OMP_PTR_TYPE(VoidPtr, Int8)
> __OMP_PTR_TYPE(VoidPtrPtr, VoidPtr)
> ```
My proposed scheme for the pointers was integrated in D79739 and will be 
commited shortly.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:234
 __OMP_RTL(__kmpc_critical, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy)
-__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, Int32)
+__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, IntPtrTy)
 __OMP_RTL(__kmpc_end_critical, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy)

fghanim wrote:
> jdoerfert wrote:
> > I think this was correct before:
> > ```
> >   KMP_EXPORT void __kmpc_critical_with_hint(ident_t *, kmp_int32 
> > global_tid, kmp_critical_name *, uint32_t hint);
> > ```
> Nop, it's supposed to be whatever `IntPtrTy` is in the frontend (i.e. 32 for 
> 32bit, 64 for 64bit).
> 
> `IntPtrTy` is actually a union with `sizety` in `CodeGenModule`
I'm confused. I copied the declaration above from the runtime. It doesn't 
contain an `IntPtrTy` or similar. I agree that `IntPtrTy` is machine dependent 
and we need your initializers. What I tried to say is that at least one 
declaration in the runtime has `__kmpc_critical_with_hint` with an int32 as 
last argument. That said, the runtime is not shy of incompatible declarations 
for functions.



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122
+  }
+}
+

fghanim wrote:
> jdoerfert wrote:
> > (I doubt we need the `initVoidPtr` function but if we do, the condition 
> > looks wrong)
> Forgot to refractor when I changed names.
> 
> Regarding `void*` , I am not sure how it is defined in fortran. That's why I 
> made it possible to initialize it separately.
I see. Let's cross that bridge once we get to it.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1013
+  return Builder.CreateCall(Fn, Args);
+}
+

fghanim wrote:
> jdoerfert wrote:
> > I think we should have a unit test for each of these.
> > 
> > Style: drop the `llvm::`, not needed.
> Aside from `CreateCopyinclauseblocks()`, I couldn't think of a unittest for 
> the others, all they do is create a runtime call based on the passed arg. 
> That's why I didn't do it already.
Let's test that one then :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim marked 5 inline comments as done.
fghanim added a comment.

This is a small patch as it is. splitting it any further would make it very 
very small :D




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:234
 __OMP_RTL(__kmpc_critical, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy)
-__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, Int32)
+__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, IntPtrTy)
 __OMP_RTL(__kmpc_end_critical, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy)

jdoerfert wrote:
> I think this was correct before:
> ```
>   KMP_EXPORT void __kmpc_critical_with_hint(ident_t *, kmp_int32 global_tid, 
> kmp_critical_name *, uint32_t hint);
> ```
Nop, it's supposed to be whatever `IntPtrTy` is in the frontend (i.e. 32 for 
32bit, 64 for 64bit).

`IntPtrTy` is actually a union with `sizety` in `CodeGenModule`



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122
+  }
+}
+

jdoerfert wrote:
> (I doubt we need the `initVoidPtr` function but if we do, the condition looks 
> wrong)
Forgot to refractor when I changed names.

Regarding `void*` , I am not sure how it is defined in fortran. That's why I 
made it possible to initialize it separately.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:594
+  // PRegPreFiniTI->getSuccessor(0) == PRegExitBB &&
+  // "Unexpected CFG structure!");
 

jdoerfert wrote:
> Preferably we would have a modified assertion. If that is too cumbersome, we 
> can probably remove it. 
I was going to remove it actually. When running Dtor over an array, clang emits 
some loop logic that makes this untrue.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1013
+  return Builder.CreateCall(Fn, Args);
+}
+

jdoerfert wrote:
> I think we should have a unit test for each of these.
> 
> Style: drop the `llvm::`, not needed.
Aside from `CreateCopyinclauseblocks()`, I couldn't think of a unittest for the 
others, all they do is create a runtime call based on the passed arg. That's 
why I didn't do it already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Thanks for these! We should probably split this in three patches.  I commented 
below, mostly minor stuff.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:101
+extern Type *IntPtrTy;
+extern Type *SizeTy;
+

I can totally see why we need `int` and `size_t` but I'm not sure about the 
others.
`void` is universally translated to `i8*` Adding a pointer to a type should be 
done in OMPKinds.def, something like:
```
#define __OMP_PTR_TYPE(NAME, BASE) OMP_TYPE(NAME, Base->getPointerTo());
__OMP_PTR_TYPE(VoidPtr, Int8)
__OMP_PTR_TYPE(VoidPtrPtr, VoidPtr)
```



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:68
+  InsertPointTy SaveIP() { return Builder.saveIP(); }
+
   /// Callback type for variable finalization (think destructors).

The above can be committed separately as "wrap IRBuilder methods" or something, 
LGTM for that part.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:371
+  /// threads copy the 'copyin' variables from Master copy to threadprivate
+  /// copies.
+  ///

Copy & paste, also below.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:234
 __OMP_RTL(__kmpc_critical, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy)
-__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, Int32)
+__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy, IntPtrTy)
 __OMP_RTL(__kmpc_end_critical, false, Void, IdentPtr, Int32, 
KmpCriticalNamePtrTy)

I think this was correct before:
```
  KMP_EXPORT void __kmpc_critical_with_hint(ident_t *, kmp_int32 global_tid, 
kmp_critical_name *, uint32_t hint);
```



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:240
+__OMP_RTL(__kmpc_threadprivate_cached, false, VoidPtr, IdentPtr, Int32, 
VoidPtr, SizeTy, Void3Ptr)
+__OMP_RTL(__kmpc_threadprivate_register, false, Void, IdentPtr, VoidPtr, 
KmpcCtorTy, KmpcCopyCtorTy, KmpcDtorTy)
+

Can you add attributes (below) for these and call them in 
`llvm/test/Transforms/OpenMP/add_attributes.ll` [if not already present]. These 
changes and the additional types should go in a separate patch.



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122
+  }
+}
+

(I doubt we need the `initVoidPtr` function but if we do, the condition looks 
wrong)



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:594
+  // PRegPreFiniTI->getSuccessor(0) == PRegExitBB &&
+  // "Unexpected CFG structure!");
 

Preferably we would have a modified assertion. If that is too cumbersome, we 
can probably remove it. 



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:930
+InsertPointTy IP, Value *MasterAddr, Value *PrivateAddr,
+llvm::IntegerType *IntPtrTy, bool BranchtoEnd) {
+  if (!IP.isSet())

Can you add a comment here, maybe some ascii art, describing what we generate.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1013
+  return Builder.CreateCall(Fn, Args);
+}
+

I think we should have a unit test for each of these.

Style: drop the `llvm::`, not needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim created this revision.
fghanim added a reviewer: jdoerfert.
Herald added subscribers: llvm-commits, cfe-commits, guansong, hiraditya, 
yaxunl.
Herald added projects: clang, LLVM.

Added new methods to generate new runtime calls
Added all required defenitions
Added some target-specific types


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79675

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/lib/Frontend/OpenMP/OMPConstants.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -93,6 +93,18 @@
 
 void OpenMPIRBuilder::initialize() { initializeTypes(M); }
 
+void OpenMPIRBuilder::setIntPtrTy(IntegerType *IntPtrTy) {
+  initializeIntPtrTy(IntPtrTy);
+}
+
+void OpenMPIRBuilder::setSizeTy(IntegerType *SizeTy) {
+  initializeSizeTy(SizeTy);
+}
+
+void OpenMPIRBuilder::setVoidPtrTy(PointerType *VoidPtrTy) {
+  initializeVoidPtrTy(VoidPtrTy);
+}
+
 void OpenMPIRBuilder::finalize() {
   for (OutlineInfo  : OutlineInfos) {
 assert(!OI.Blocks.empty() &&
@@ -576,9 +588,10 @@
  "Unexpected finalization stack state!");
 
   Instruction *PRegPreFiniTI = PRegPreFiniBB->getTerminator();
-  assert(PRegPreFiniTI->getNumSuccessors() == 1 &&
- PRegPreFiniTI->getSuccessor(0) == PRegExitBB &&
- "Unexpected CFG structure!");
+
+  //  assert(PRegPreFiniTI->getNumSuccessors() == 1 &&
+  // PRegPreFiniTI->getSuccessor(0) == PRegExitBB &&
+  // "Unexpected CFG structure!");
 
   InsertPointTy PreFiniIP(PRegPreFiniBB, PRegPreFiniTI->getIterator());
   FiniCB(PreFiniIP);
@@ -912,6 +925,93 @@
   ExitCall->getIterator());
 }
 
+OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::CreateCopyinClauseBlocks(
+InsertPointTy IP, Value *MasterAddr, Value *PrivateAddr,
+llvm::IntegerType *IntPtrTy, bool BranchtoEnd) {
+  if (!IP.isSet())
+return IP;
+
+  IRBuilder<>::InsertPointGuard IPG(Builder);
+
+  BasicBlock *OMP_Entry = IP.getBlock();
+  Function *CurFn = OMP_Entry->getParent();
+  BasicBlock *CopyBegin =
+  BasicBlock::Create(M.getContext(), "copyin.not.master", CurFn);
+  BasicBlock *CopyEnd = nullptr;
+  // If entry block is terminated, split to preserve the branch to following
+  // basic block, otherwise, leave everything as is.
+  if (isa_and_nonnull(OMP_Entry->getTerminator())) {
+CopyEnd = OMP_Entry->splitBasicBlock(OMP_Entry->getTerminator(),
+ "copyin.not.master.end");
+OMP_Entry->getTerminator()->eraseFromParent();
+  } else {
+CopyEnd =
+BasicBlock::Create(M.getContext(), "copyin.not.master.end", CurFn);
+  }
+
+  Builder.SetInsertPoint(OMP_Entry);
+  Value *MasterPtr = Builder.CreatePtrToInt(MasterAddr, IntPtrTy);
+  Value *PrivatePtr = Builder.CreatePtrToInt(PrivateAddr, IntPtrTy);
+  Value *cmp = Builder.CreateICmpNE(MasterPtr, PrivatePtr);
+  Builder.CreateCondBr(cmp, CopyBegin, CopyEnd);
+
+  Builder.SetInsertPoint(CopyBegin);
+  if (BranchtoEnd) {
+BranchInst *br = Builder.CreateBr(CopyEnd);
+Builder.SetInsertPoint(br);
+  }
+
+  return Builder.saveIP();
+}
+
+CallInst *OpenMPIRBuilder::CreateOMPAlloc(const LocationDescription ,
+  Value *Size, Value *Allocator,
+  std::string Name) {
+  IRBuilder<>::InsertPointGuard IPG(Builder);
+  Builder.restoreIP(Loc.IP);
+
+  Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
+  Value *Ident = getOrCreateIdent(SrcLocStr);
+  Value *ThreadId = getOrCreateThreadID(Ident);
+  Value *Args[] = {ThreadId, Size, Allocator};
+
+  Function *Fn = getOrCreateRuntimeFunction(OMPRTL___kmpc_alloc);
+
+  return Builder.CreateCall(Fn, Args, Name);
+}
+
+CallInst *OpenMPIRBuilder::CreateOMPFree(const LocationDescription ,
+ Value *Addr, Value *Allocator,
+ std::string Name) {
+  IRBuilder<>::InsertPointGuard IPG(Builder);
+  Builder.restoreIP(Loc.IP);
+
+  Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
+  Value *Ident = getOrCreateIdent(SrcLocStr);
+  Value *ThreadId = getOrCreateThreadID(Ident);
+  Value *Args[] = {ThreadId, Addr, Allocator};
+  Function *Fn = getOrCreateRuntimeFunction(OMPRTL___kmpc_free);
+  return Builder.CreateCall(Fn, Args, Name);
+}
+
+CallInst *OpenMPIRBuilder::CreateCachedThreadPrivate(
+const LocationDescription , llvm::Value *Pointer,
+llvm::ConstantInt *Size, const llvm::Twine ) {
+  IRBuilder<>::InsertPointGuard IPG(Builder);
+  Builder.restoreIP(Loc.IP);
+
+  Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
+  Value *Ident = getOrCreateIdent(SrcLocStr);
+  Value