[Openmp-commits] [PATCH] D62199: [OPENMP][NVPTX]Fix barriers and parallel level counters, NFC.

[Hal Finkel via Phabricator via Openmp-commits]

hfinkel added a comment.

In D62199#1513126 <https://reviews.llvm.org/D62199#1513126>, @Hahnfeld wrote:

> In D62199#1512697 <https://reviews.llvm.org/D62199#1512697>, @arsenm wrote:
>
> > In D62199#1512638 <https://reviews.llvm.org/D62199#1512638>, @ABataev wrote:
> >
> > > 1. Because it is impossible to test them, that why it is NFC.
> >
> >
> > This doesn't make it NFC. There is intended change in behavior.
>
>
> +1. NFC = no functional change, and apparently there are changes and they are noticeable.
>
> >> 2. Yes, this is true. Bu the changes are rather small.
> > 
> > This isn't a reason to merge unrelated patches
>
> +1
>
> >> 3. It is only your opinion.
> > 
> > I agree with this issue. This is very hand wavy. If you cannot describe what the undefined behavior or problem is is, you can't come up with a sensible solution. As presented here, this does not justify why this fixes or works around the issue.
> > 
> >> 4. Again, this is your problem, The patch was approved bythe code owner
> > 
> > Patches can still be subject to post-commit reviews
>
> Multiple things that bother me:
>
> 1. I'm not sure that @grokos is indeed the code owner. There was a proposal, but it was never formally accepted. Given that he's now working for a different vendor, does it make sense for him to be owner for code related to Nvidia code?


There need not be a relationship between one's employer and code ownership, even for backends.

> 2. According to the Developer Policy <https://llvm.org/docs/DeveloperPolicy.html#code-owners>:
> 
>> Note that code ownership is completely different than reviewers: anyone can review a piece of code, and we welcome code review from anyone who is interested. Code owners are the “last line of defense” to guarantee that all patches that are committed are actually reviewed.
> 
> So saying "this is your problem" to a valid review is not appropriate IMO.

That's correct: the response is inappropriate.

Code owners serve as reviewers of last resort. The entire project is run using a consensus-driven review process, and all interested parties are included. A code owner might take a leadership role in forming that consensus, but all code is subject to consensus-driven review both pre- and post-commit. Consensus does not necessarily mean unanimous agreement, but in this case there is clearly broad disagreement among the reviewers regarding the combining of small changes, the title of the commit and level of explanation regarding the lack of tests, and so on. These concerns should be addressed.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62199