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

Fri May 24 02:56:33 PDT 2019

grokos added a comment.

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. D56274 <https://reviews.llvm.org/D56274> is preventing LLVM from applying an optimization which can break `syncthreads()`. It replaces `syncthreads()` with the equivalent asm instruction. Apart from that bit (which doesn't change functionality, `syncthreads()` and the asm intrinsic are equivalent), the resulting code is identical and that's why the patch is marked as NFC. D61523 <https://reviews.llvm.org/D61523> was superseded by D61801 <https://reviews.llvm.org/D61801> (if you noticed, it hasn't been committed and I guess the author should abandon it). D61526 <https://reviews.llvm.org/D61526> is an optimization of the thread-limit counter. It used to be a field in `TaskDesc_items` (and reside in global memory) and now it has been turned into a shared variable (residing in shared memory). The variable is accessed faster, but its functionality is the very same. From this perspective, I am OK with the NFC tag for that patch. Yes, it is a gray area once again as with volatile and no absolute answer could be given. D61785 <https://reviews.llvm.org/D61785> does the same with the threads-in-team counter and D61801 <https://reviews.llvm.org/D61801> does the same with the threads-in-parallel-region counter. For these 2 last patches you are right that the patch description and the commit messages are unclear or even wrong; D61785 <https://reviews.llvm.org/D61785> should state explicitly that we are taking about the threads-in-team counter (instead of a vague "thread counter"), whereas in D61801 <https://reviews.llvm.org/D61801> "handling of thread limit" is plain wrong and should be replaced with "handling of threads-in-parallel-region counter". I can edit the commit messages for the sake of clarity and consistency.

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

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

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

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62199