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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62199