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

Wed Jun 10 15:02:04 PDT 2020

jdoerfert added subscribers: raghavendhra, jhuber6, anchu-rajendran, kiranktp, DavidTruby, ronlieb, kiranchandramohan.
jdoerfert added a comment.

In D79675#2058657 <https://reviews.llvm.org/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 <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.

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

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

I'm not even sure I follow. D80222 <https://reviews.llvm.org/D80222> landed, as far as I can tell. Can you elaborate on these suggestions so I do not misinterpret them?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675