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

Fri May 24 02:06:15 PDT 2019

ABataev added a comment.

In D62199#1515182 <https://reviews.llvm.org/D62199#1515182>, @jdoerfert wrote:

> In D62199#1515159 <https://reviews.llvm.org/D62199#1515159>, @grokos wrote:
>
> > Let me address what has been written so far from my perspective and request some changes since there has been demand for them:
> >
> > 1. 1, 4 and 5 represent my point of view, which is of course subjective, anyone could have a different mentality. But calling my reviews "reviews" and my activity here "suspicious" doesn't contribute at all. In pretty much the same way the "this is your problem, I've got the owner's approval" reply is inappropriate (and in no way do I endorse this "argument" as a reply to reviewers' requests).
>
>
> I was not only referring to you, but it was not the right choice of words to begin with, you're right about that.
>  I will stick to arguments from now on. What I should have said is:
>
> I think the following "NFC" commits are not "NFC" or the commit message is not matching: D61801 <https://reviews.llvm.org/D61801>, D61785 <https://reviews.llvm.org/D61785>, D61526 <https://reviews.llvm.org/D61526>, D61523 <https://reviews.llvm.org/D61523>, D56274 <https://reviews.llvm.org/D56274>
>  This is one of the commits that explain the problem they fix but do not provide a test: D56332 <https://reviews.llvm.org/D56332>
>  This commit message isn't explanatory and doesn't match the patch: D56290 <https://reviews.llvm.org/D56290>
>
> And I am concerned because really quick LGTM for non-trivial changes happened before (not including this one or any of the aforementioned), e.g.,
>  in D53141 <https://reviews.llvm.org/D53141> @Hahnfeld pointed out problems.

These patches are really NFC.  They do not add a new functionality, they simplify the existing one. And we already have the tests for these changes in NVPTX part of the libomptarget. These tests are not affected by these patches. That's why they are NFC.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62199