[OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[fghanim]

Summary

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

[jdoerfert]

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

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

101 ↗

(On Diff #263030)

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)

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

63

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

349

Copy & paste, also below.

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

234 ↗

(On Diff #263030)

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

240 ↗

(On Diff #263030)

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.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

122 ↗

(On Diff #263030)

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

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

616

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

950

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

1033

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

Style: drop the llvm::, not needed.

[fghanim]

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

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

234 ↗

(On Diff #263030)

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

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

616

I was going to remove it actually. When running Dtor over an array, clang emits some loop logic that makes this untrue.

1033

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.

[fghanim]

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

122 ↗

(On Diff #263030)

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.

[jdoerfert]

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.

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

101 ↗

(On Diff #263030)

My proposed scheme for the pointers was integrated in D79739 and will be commited shortly.

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

234 ↗

(On Diff #263030)

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.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

122 ↗

(On Diff #263030)

I see. Let's cross that bridge once we get to it.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

1033

Let's test that one then :)

[fghanim]

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

101 ↗

(On Diff #263030)

Kept a couple a Ineed in OMPConstants. will add them in OMPKinds.def after the other patch lands.

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

234 ↗

(On Diff #263030)

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.

240 ↗

(On Diff #263030)

Added in D79739

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

122 ↗

(On Diff #263030)

Modified it a bit and removed usage from initialization. If fortran people don't end using it, then we can remove it.

[fghanim]

• removed many Definitions from OMPKinds.def due to being added in D79739
• made changes based on reviewer comments
• added unit test for CreateCopyinClauseBlocks()

[jdoerfert]

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?

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

318

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

369

Style: Use Name for the variable name.

371

Nit: Copy and paste

382

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

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

234 ↗

(On Diff #263030)

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

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

69 ↗

(On Diff #264006)

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?

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

140

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

966

Great! thanks!

995

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

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

817

Great.

[fghanim]

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.

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

234 ↗

(On Diff #263030)

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.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

69 ↗

(On Diff #264006)

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?!

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

140

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.

[jdoerfert]

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.

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

234 ↗

(On Diff #263030)

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.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

69 ↗

(On Diff #264006)

Yes, second (and third) argument. Given that every user has to initialize all the types we can just make it a single call.

122 ↗

(On Diff #263030)

Let's not add stuff that might be used.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

140

Point taken, though I'm fine with criticizing my own design :)

My suggestion now: merge everything into the OpenMPIRBuilder::initialize we already have. I'm also fine with removing this and letting users call (a single) initializeTypes, thought it is a bit harder to "know" that is needed.

[jdoerfert]

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

[fghanim]

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

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

234 ↗

(On Diff #263030)

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.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

122 ↗

(On Diff #263030)

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.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

140

I think either way is fine, so I wasn't criticizing. I was explaining why I did what I did. :)

Anyways, I made the change.

[fghanim]

Addressing reviewer Comments

[jdoerfert]

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.

[fghanim]

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.

[jdoerfert]

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.

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

58

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

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

86 ↗

(On Diff #265071)

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

109 ↗

(On Diff #265071)

I guess we don't need anymore.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

138

Probably won't need these either.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

61 ↗

(On Diff #265071)

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

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

65

Same as above, also below.

[fghanim]

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

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

86 ↗

(On Diff #265071)

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.

[fghanim]

llvm/lib/Frontend/OpenMP/OMPConstants.cpp

86 ↗

(On Diff #265071)

I meant to say the patch has *not* landed yet. sorry for the confusion.

[fghanim]

Addressing more reviewers comments.

[jdoerfert]

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 createXXXX 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 createXXXX 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 createXXXX 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.XXXX 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.

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

62

Unused?

[fghanim]

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 createXXXX 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 up 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

[[edited for brevity]]
You wasted my time twice - and this is not my opinion of how things happened, it's something you can look back and see for yourself:
1- You asked me to modify the void* and add the pointer macros, only then to ask someone else on the same day to make the same exact changes, without letting me know that you did, which meant i started to work on it. when they got finished 1 day before me, you tell me to drop the changes because we have them somewhere else.
2- You wasted my time again with the target specific types. We explicitly said this time on both patches, that I am to work on these. You and I spend over a week working on those in back and forth. Only to yesterday when the same person above ends implementing very similar changes to what we worked out, and then here you come again and ask me to drop the changes again.

So based on that, I pose the following 2 question to you

• How come after we explicitly decided that I am to implement the pointer macros , and then again target-specific types, based on comments you made to make each change in a very specific way, you ask other people to make the same change after me being done with it?
• Since you keep bringing it up. With all honesty, how do you believe splitting the typing in a separate patch would have saved my time, when here I am being told to drop the changes anyway? Also, please tell me how everything that happened here wasn't a complete and utter waste of my time?

Here is the bottom line, and this will be the end for me of this back and forth - we are not getting anywhere. Thank you for your responses, and for your valuable comments. I appreciate that you are really busy - I've seen it on two different occasions. So, I really do appreciate your comments - I genuinely mean it. :)
But remember, I also have important things I need to work on, so all I ask is please be mindful of my time, The events that transpired with this patch makes me think you are not. If you don't want me to make a change don't ask me to do it, if you ask me to do it, and I do it, then don't let that effort go to waste. If you change your mind about who does what, Let me KNOW ASAP

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

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

101 ↗

(On Diff #263030)

Here is the thing - it is on me to make sure this patch works, I will happily drop all changes to OMPConstants.h/cpp ,OMPKinds.def, CGModule, and unittests here, and will keep the changes on my local copy, on the condition that you guys will handle any problems that arise with types in this patch. I am NOT going to fix anything here related to typing - I've wasted enough time on this.

Alternatively, we keep my changes. and later on, we make another patch that cleans up both this and D80222.

If neither is agreeable, then we have a problem.

If you have any other comments, on other things, I'll happily work on those.

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

62

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?

[jdoerfert]

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.

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

62

Generally we should not include code we don't need (or that is not tested).

[fghanim]

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?

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

62

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?

[fghanim]

• Rebase + refactor based on D80222
• addressed reviewer comments

[jdoerfert]

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?

[fghanim]

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?

[fghanim]

Ping.
Does this patch need further changes?

[jdoerfert]

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

[fghanim]

commited: rG82b8236cf248

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

 //===- IR/OpenMPIRBuilder.h - OpenMP encoding builder for LLVM IR - C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
public:
   ///                  should be placed.
   ///
   /// A finalize callback knows about all objects that need finalization, e.g.
   /// destruction, when the scope of the currently generated construct is left
   /// at the time, and location, the callback is invoked.
   using FinalizeCallbackTy = std::function<void(InsertPointTy CodeGenIP)>;
 
   struct FinalizationInfo {
     /// The finalization callback provided by the last in-flight invocation of
     /// CreateXXXX for the directive of kind DK.
     FinalizeCallbackTy FiniCB;
 
     /// The directive kind of the innermost directive that has an associated
     /// region which might require finalization when it is left.
     omp::Directive DK;
 
     /// Flag to indicate if the directive is cancellable.
     bool IsCancellable;
   };
 
   /// Push a finalization callback on the finalization stack.
   ///
public:
   /// \param BodyGenCB Callback that will generate the region code.
   /// \param FiniCB Callback to finalize variable copies.
   ///
   /// \returns The insertion position *after* the master.
   InsertPointTy CreateMaster(const LocationDescription &Loc,
                              BodyGenCallbackTy BodyGenCB,
                              FinalizeCallbackTy FiniCB);
 
-  /// Generator for '#omp master'
+  /// Generator for '#omp critical'
   ///
   /// \param Loc The insert and source location description.
   /// \param BodyGenCB Callback that will generate the region body code.
   /// \param FiniCB Callback to finalize variable copies.
   /// \param CriticalName name of the lock used by the critical directive
   /// \param HintInst Hint Instruction for hint clause associated with critical
   ///
   /// \returns The insertion position *after* the master.
   InsertPointTy CreateCritical(const LocationDescription &Loc,
                                BodyGenCallbackTy BodyGenCB,
                                FinalizeCallbackTy FiniCB,
                                StringRef CriticalName, Value *HintInst);
 
+  /// Generate conditional branch and relevant BasicBlocks through which private
+  /// threads copy the 'copyin' variables from Master copy to threadprivate
+  /// copies.
+  ///
+  /// \param IP insertion block for copyin conditional
+  /// \param MasterVarPtr a pointer to the master variable
+  /// \param PrivateVarPtr a pointer to the threadprivate variable
+  /// \param IntPtrTy Pointer size type
+  /// \param BranchtoEnd Create a branch between the copyin.not.master blocks
+  //         and copy.in.end block
+  ///
+  /// \returns The insertion point where copying operation to be emitted.
+  InsertPointTy CreateCopyinClauseBlocks(InsertPointTy IP, Value *MasterAddr,
+                                         Value *PrivateAddr,
+                                         llvm::IntegerType *IntPtrTy,
+                                         bool BranchtoEnd = true);
+
+  /// Create a runtime call for kmpc_Alloc
+  ///
+  /// \param Loc The insert and source location description.
+  /// \param Size Size of allocated memory space
+  /// \param Allocator Allocator information instruction
+  /// \param Name Name of call Instruction for OMP_alloc
+  ///
+  /// \returns CallInst to the OMP_Alloc call
+  CallInst *CreateOMPAlloc(const LocationDescription &Loc, Value *Size,
+                           Value *Allocator, std::string Name = "");
+
+  /// Create a runtime call for kmpc_free
+  ///
+  /// \param Loc The insert and source location description.
+  /// \param Addr Address of memory space to be freed
+  /// \param Allocator Allocator information instruction
+  /// \param Name Name of call Instruction for OMP_Free
+  ///
+  /// \returns CallInst to the OMP_Free call
+  CallInst *CreateOMPFree(const LocationDescription &Loc, Value *Addr,
+                          Value *Allocator, std::string Name = "");
+
+  /// Create a runtime call for kmpc_threadprivate_cached
+  ///
+  /// \param Loc The insert and source location description.
+  /// \param Pointer pointer to data to be cached
+  /// \param Size size of data to be cached
+  /// \param Name Name of call Instruction for callinst
+  ///
+  /// \returns CallInst to the thread private cache call.
+  CallInst *CreateCachedThreadPrivate(const LocationDescription &Loc,
+                                      llvm::Value *Pointer,
+                                      llvm::ConstantInt *Size,
+                                      const llvm::Twine &Name = Twine(""));
+
 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.
EmitOMPInlinedRegion(omp::Directive OMPD, Instruction *EntryCall,
                        Instruction *ExitCall, BodyGenCallbackTy BodyGenCB,
                        FinalizeCallbackTy FiniCB, bool Conditional = false,
                        bool HasFinalize = true);
 
   /// Get the platform-specific name separator.
   /// \param Parts different parts of the final name that needs separation
   /// \param FirstSeparator First separator used between the initial two
   ///        parts of the name.
-  /// \param Separator separator used between all of the rest consecutinve
+  /// \param Separator separator used between all of the rest consecutive
   ///        parts of the name
   static std::string getNameWithSeparators(ArrayRef<StringRef> Parts,
                                            StringRef FirstSeparator,
                                            StringRef Separator);
 
   /// Gets (if variable with the given name already exist) or creates
   /// internal global variable with the specified Name. The created variable has
   /// linkage CommonLinkage by default and is initialized by null value.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

 //===- OpenMPIRBuilder.cpp - Builder for LLVM-IR for OpenMP directives ----===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 /// \file

 void OpenMPIRBuilder::finalize() {
   for (OutlineInfo &OI : OutlineInfos) {
     assert(!OI.Blocks.empty() &&
            "Outlined regions should have at least a single block!");
     BasicBlock *RegEntryBB = OI.Blocks.front();
     Function *OuterFn = RegEntryBB->getParent();
     CodeExtractorAnalysisCache CEAC(*OuterFn);
     CodeExtractor Extractor(OI.Blocks, /* DominatorTree */ nullptr,
                             /* AggregateArgs */ false,
                             /* BlockFrequencyInfo */ nullptr,
                             /* BranchProbabilityInfo */ nullptr,
                             /* AssumptionCache */ nullptr,
                             /* AllowVarArgs */ true,
                             /* AllowAlloca */ true,
                             /* Suffix */ ".omp_par");
 
     LLVM_DEBUG(dbgs() << "Before     outlining: " << *OuterFn << "\n");
     assert(Extractor.isEligible() &&
            "Expected OpenMP outlining to be possible!");
IRBuilder<>::InsertPoint OpenMPIRBuilder::CreateParallel(
   // finalize function a last time to finalize values between the pre-fini
   // block and the exit block if we left the parallel "the normal way".
   auto FiniInfo = FinalizationStack.pop_back_val();
   (void)FiniInfo;
   assert(FiniInfo.DK == OMPD_parallel &&
          "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);
 
   SmallPtrSet<BasicBlock *, 32> ParallelRegionBlockSet;
   SmallVector<BasicBlock *, 32> Worklist;
   ParallelRegionBlockSet.insert(PRegEntryBB);
   ParallelRegionBlockSet.insert(PRegExitBB);
 
   // Collect all blocks in-between PRegEntryBB and PRegExitBB.
   Worklist.push_back(PRegEntryBB);
OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::emitCommonDirectiveExit(
   // place the Exitcall as last instruction before Finalization block terminator
   ExitCall->removeFromParent();
   Builder.Insert(ExitCall);
 
   return IRBuilder<>::InsertPoint(ExitCall->getParent(),
                                   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<BranchInst>(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 &Loc,
+                                          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 = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_alloc);
+
+  return Builder.CreateCall(Fn, Args, Name);
+}
+
+CallInst *OpenMPIRBuilder::CreateOMPFree(const LocationDescription &Loc,
+                                         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 = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_free);
+  return Builder.CreateCall(Fn, Args, Name);
+}
+
+CallInst *OpenMPIRBuilder::CreateCachedThreadPrivate(
+    const LocationDescription &Loc, llvm::Value *Pointer,
+    llvm::ConstantInt *Size, const llvm::Twine &Name) {
+  IRBuilder<>::InsertPointGuard IPG(Builder);
+  Builder.restoreIP(Loc.IP);
+
+  Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
+  Value *Ident = getOrCreateIdent(SrcLocStr);
+  Value *ThreadId = getOrCreateThreadID(Ident);
+  Constant *ThreadPrivateCache =
+      getOrCreateOMPInternalVariable(Int8PtrPtr, Name);
+  llvm::Value *Args[] = {Ident, ThreadId, Pointer, Size, ThreadPrivateCache};
+
+  Function *Fn =
+      getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_threadprivate_cached);
+
+  return Builder.CreateCall(Fn, Args);
+}
+
 std::string OpenMPIRBuilder::getNameWithSeparators(ArrayRef<StringRef> Parts,
                                                    StringRef FirstSeparator,
                                                    StringRef Separator) {
   SmallString<128> Buffer;
   llvm::raw_svector_ostream OS(Buffer);
   StringRef Sep = FirstSeparator;
   for (StringRef Part : Parts) {
     OS << Sep << Part;

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

 //===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
protected:
   std::unique_ptr<Module> M;
   Function *F;
   BasicBlock *BB;
   DebugLoc DL;
 };
 
 TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
   OpenMPIRBuilder OMPBuilder(*M);
   OMPBuilder.initialize();
 
   IRBuilder<> Builder(BB);
 
   OMPBuilder.CreateBarrier({IRBuilder<>::InsertPoint()}, OMPD_for);
   EXPECT_TRUE(M->global_empty());
   EXPECT_EQ(M->size(), 1U);
   EXPECT_EQ(F->size(), 1U);
   EXPECT_EQ(BB->size(), 0U);
TEST_F(OpenMPIRBuilderTest, CriticalDirective) {
   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);
 }
 
+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<BranchInst>(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<CmpInst>(EntryBr->getCondition());
+
+  EXPECT_NE(CMP, nullptr);
+  EXPECT_NE(NotMasterBB, nullptr);
+  EXPECT_NE(CopyinEnd, nullptr);
+
+  BranchInst* NotMasterBr = dyn_cast_or_null<BranchInst>(NotMasterBB->getTerminator());
+  EXPECT_NE(NotMasterBr, nullptr);
+  EXPECT_FALSE(NotMasterBr->isConditional());
+  EXPECT_EQ(CopyinEnd,NotMasterBr->getSuccessor(0));
+}
+
 } // namespace