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

[Fady Ghanim via Phabricator via llvm-commits]

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

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. 
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 <https://reviews.llvm.org/D79676> and D79677 <https://reviews.llvm.org/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 <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

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