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

Alexey Bataev via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri May 24 20:24:45 PDT 2019


ABataev added a comment.

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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62199





More information about the llvm-commits mailing list