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

[Fady Ghanim via Phabricator via llvm-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 <https://reviews.llvm.org/D80222> moments after it was uploaded. Now, D79675 <https://reviews.llvm.org/D79675> and D79676 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D79739> has been merged with D80222 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D79676> after D80222 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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