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

Fri May 24 18:49:16 PDT 2019

jdoerfert added a comment.

In D62199#1515511 <https://reviews.llvm.org/D62199#1515511>, @grokos wrote:

> In D62199#1515182 <https://reviews.llvm.org/D62199#1515182>, @jdoerfert wrote:
>
> >
>
>
> I agree with some of your complaints, I disagree with others:
>
> > 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>
>
> I think all of them are NFC.

I do not. Maybe we disagree what NFC is supposed to mean.
As I mentioned for this commit, I don't think a "FIX" can be "NFC". I also do not consider optimizations as NFC.
If something improves "performance" and "shared memory", it has to change functionality.

>> This is one of the commits that explain the problem they fix but do not provide a test: D56332 <https://reviews.llvm.org/D56332>
> 
> True, a test should be added. I was more concerned with fixing the dynamic scheduling bug, that was the important bit, but nonetheless that change should be accompanied by a corresponding dynamic scheduling test (especially since we now have a testing infrastructure in libomptarget).

I don't like the argument "we need to fix a bug first", neither for this patch nor for the one in the comment above.

>> This commit message isn't explanatory and doesn't match the patch: D56290 <https://reviews.llvm.org/D56290>
> 
> That's just a code polishing patch, I don't know what more the author could describe about it. He did some refactoring and cleanup, marked functions as inline and/or const and/or static etc. OK, just saying "Formatting" in the description may look poor, but then again there's the spirit of the law I mentioned earlier. I wouldn't make a big deal out of a poor message for such a patch.

I see a difference between "formating" and this patch. Functions are removed, code rewritten, a comment about a deadlock was removed because it seemed the problem might have been something else and it works now. That is not only more than formating but another example of a change that should not be rubber-stamped.

>> 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.
> 
> You are right, there was an issue I didn't catch. I never claimed to be infallible and no one is, that's why we like to have more than one pair of eyes review a patch. There are issues you may catch and I may miss and vice versa.

I don't want anyone to be infallible, nor do I want to point out every mistake made by a person. This is not personal to me and I don't want you to get that impression.
What I want is to discuss a systematic problem in the review process. I don't claim this is intentional, but I claim there is *at least* one.

> Usually, if I don't have any comments about a patch I wait a day or two before accepting it, so that I give time to other reviewers to speak up.

One problem is that it is not a day or two before accepting (see below). Another problem is that comments by others are not always taken into account (https://reviews.llvm.org/D51875#1229582), e.g., by checking in if they have been resolved (D60918 <https://reviews.llvm.org/D60918>).

Time (hours:minutes) between upload to LGTM for the patches I mentioned earlier:

D62199 <https://reviews.llvm.org/D62199>  3:30
D61801 <https://reviews.llvm.org/D61801>  6:30
D61785 <https://reviews.llvm.org/D61785>  4:30
D61526 <https://reviews.llvm.org/D61526>  0:50
D61523 <https://reviews.llvm.org/D61523>  2:00
D56274 <https://reviews.llvm.org/D56274>  0:15
D56332 <https://reviews.llvm.org/D56332>  6:30
D56290 <https://reviews.llvm.org/D56290> 19:06 (ping after 17:30)
D53141 <https://reviews.llvm.org/D53141>  0:21 (objection 6m later)

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62199