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

Fri May 22 15:34:13 PDT 2020

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 <https://reviews.llvm.org/D79675#2051079>, @jdoerfert wrote:

> In D79675#2047154 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D70109> , which -I can only assume- fit yours and the community's criteria? ;)

> With D80222 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 how we initialized target-specific types.
- weekend.
- On the 19th, I uploaded a new patch based on your comments regarding target-specific types, after we exchanged some comments on same day. In one of them you seemed to suggest I am causing delay to D80222 <https://reviews.llvm.org/D80222> which was created that same day
- On the 20th, You comment, and say maybe I should **use `module datalayout` to handle `SizeTy` initialization** in tests. I upload a patch based on these comments same day at noon.
- On the 21st, A change is made to D80222 <https://reviews.llvm.org/D80222> that makes all my work the previous week obsolete using the same method we discussed i.e. `module datalayout`
- Today, you ask me to drop the changes I made based on your comments over the last 10 days working on.

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, both times, the same exact change suddenly pops up in a different patch 2 days later 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".

================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:101
+extern Type *IntPtrTy;
+extern Type *SizeTy;
+
----------------
fghanim wrote:
> 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.
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.

================
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:
> 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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675