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

Sat May 25 14:27:00 PDT 2019

ABataev added a comment.

In D62199#1517419 <https://reviews.llvm.org/D62199#1517419>, @hfinkel wrote:

> In D62199#1517258 <https://reviews.llvm.org/D62199#1517258>, @ABataev wrote:
>
> > In D62199#1517150 <https://reviews.llvm.org/D62199#1517150>, @hfinkel wrote:
> >
> > > In D62199#1517144 <https://reviews.llvm.org/D62199#1517144>, @ABataev wrote:
> > >
> > > > In D62199#1517099 <https://reviews.llvm.org/D62199#1517099>, @hfinkel wrote:
> > > >
> > > > > In D62199#1517095 <https://reviews.llvm.org/D62199#1517095>, @ABataev wrote:
> > > > >
> > > > > > In D62199#1517078 <https://reviews.llvm.org/D62199#1517078>, @hfinkel wrote:
> > > > > >
> > > > > > > In D62199#1517071 <https://reviews.llvm.org/D62199#1517071>, @arsenm wrote:
> > > > > > >
> > > > > > > > In D62199#1515385 <https://reviews.llvm.org/D62199#1515385>, @ABataev wrote:
> > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > 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.
> > > > > > > >
> > > > > > > > This seems like a misunderstanding of the intent of NFC to me. NFC is for commits that are not supposed to have any observable effect, like refactoring. Anything that fixes a problem, while not providing what might be considered a feature or new functionality, is not NFC
> > > > > > >
> > > > > > >
> > > > > > > @arsenm is certainly correct. The developer policy speaks to this as well, stating, "Avoid committing formatting- or whitespace-only changes outside of code you plan to make subsequent changes to. Also, try to separate formatting or whitespace changes from functional changes, either by correcting the format first (ideally) or afterward. Such changes should be highly localized and the commit message should clearly state that the commit is not intended to change functionality, usually by stating it is NFC." The policy does not explicitly state that this is the only valid use of NFC, and it is sometimes used for refactoring as well, but if a bug is fixed, we don't consider that to be NFC (because the effective functionality of the product has changed).
> > > > > >
> > > > > >
> > > > > > In some cases a "fix" can be NFC, if it fixes the bug in the implementation of the non-functional requirement, for example.
> > > > > >  Functional changes - the changes that change functionality, i.e. change the expected output for the subset of inputs. Generally speaking, "volatile" qualifier does not change the logical dependency between inputs and expected outputs, it changes how the compiler handles such variables and optimizes them. But the logic remains the same.
> > > > > >  I'm not sure that some internal compiler/programming language requirements, can be considered as functional. 
> > > > > >  Functional requirements are supported by non-functional requirements (also known as "quality requirements"), which impose constraints on the design or implementation. In this case, "volatile" modifier can be considered as the implementation constraint.
> > > > >
> > > > >
> > > > > I understand what you're saying, but that's not how the LLVM community uses the term NFC. One way to think about it is the following: If any change in the behavior of the system: functionality, correctness, performance (at compile time or run time), could be bisected to this commit, then it's not NFC. NFC is for things like reformatting code - unless a mistake was made, no observable behavior of the system should bisect to a commit that only reformats code. In some cases, *small* refactorings are labeled as NFC. If it were to turn out that a refactoring happened to introduce a compile-time-performance change, for example, then labeling the commit as NFC would have been in error (likely accidentally). Thus, as the LLVM community uses the term NFC, a "fix" can never be NFC.
> > > >
> > > >
> > > > Let me disagree with you. At first, you said "The policy does not explicitly state that this is the only valid use of NFC, and it is sometimes used for refactoring as well." Now you're saying that this only for formatting.
> > > >  Functionality is well determined term. I don't think there is a different meaning of it in the community.
> > >
> > >
> > > "Now you're saying that this only for formatting." - I specifically did not say that. I did specifically say, "it is sometimes used for refactoring as well." The functionality of the compiler and its runtime libraries is changed if a bug is fixed, Alexey. I don't believe there's disagreement in the community on that point.
> >
> >
> > Hal, again, you arguing against fotmal definition of functional and non-functional, functional requirement and non-functional requirement, in the computer science. If the "fix" fixes the implementation of the non-functional requirement, it can be a non-functional fix. 
> >  I'm not saying that you cannot redefine those formal definitions and use your own. You absolutely can do this! But you need to define the new meaning of those old terms strictly and introduce the new terms for the redfined ones. Only after that we can use these terms with their new meanings.
>
>
> Alexey, I feel like we're talking past each other. Tagging commits as NFC is not a theoretical exercise, it is intended to be a practically-usable note to those reviewing the commit history that, "this commit shouldn't have changed any observable behavior of the system.".  When you say that a commit "they simplify the existing [functionality]", that sounds like refactoring, and that could very well be NFC. I don't think we're in disagreement on that. The disagreement seems to be over things like this:
>
> >   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 we have several underlying issues. It sounds like you're saying that you've never observed a difference between using the intrinsic and the inline asm, but you're changing to the inline asm because the intrinsic is "known to be buggy." Thus, while you've never observed the bug yourself in this context, the new form simply seems safer than the old form. Is that correct? I can see an argument for marking such a change as NFC, but should someone compile the code with a compiler and in some configuration where the bug does manifest, it would not be NFC *for them*. Because this risk exists, we should not mark the change as NFC. If you have observed the bug, then the change is certainly not NFC (because the behavior of the system changed from before to after the commit, at least for some system configurations). We also shouldn't tag commits adding volatile qualifiers as NFC for the same reason (you're doing this because the commit might change the behavior of the system for someone, even if the exact set of such configurations is unknown).

Hal, you don't see a difference between functional and non-functional requirements. The fact, that behavior ID changed because of the optimization,  is the implementation constraint. It is non-functional requirement at the moment to use the asm form instead of the intrinsic. Because, there is no logical changes in the dependency between inputs and outputs. These 2 forms has the same functionality. Thus, it is non-functional requirement to use the second form instead of the original one at the moment. And because of that this is a non-functional change, because the logical dependency between inputs and outputs remains the same it was before.
The same is with volatile. This is just a compiler constraint to use this modifier, this not a functional requirement. And thus the change can be marked as non-functional.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62199