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

[Hal Finkel via Phabricator via Openmp-commits]

hfinkel added a comment.

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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62199