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

Fri May 22 08:01:44 PDT 2020

jdoerfert added a comment.

In D79675#2047154 <https://reviews.llvm.org/D79675#2047154>, @fghanim wrote:

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

----

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 insertion point information
+  InsertPointTy SaveIP() { return Builder.saveIP(); }
+
----------------
Unused?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675