"[NFC]" Abuse

Hi all

Twice in the last week I've had to bisect crashes in the middle end or failed CI
due to commits marked "[NFC]" that changed something fundamental about a public
API or the format of IR.

While I understand LLVM's always been pretty fluid with API and ABI stability,
it smacks a little when the offending commit is marked "[NFC]".

Can some elders / code-owners comment on the expected threshold for what no
longer counts as "NFC"? I'd personally like to limit its usage to things like
changing a local variable name, rewording a comment, or clang-formatting
something - not API, ABI, or IR changes.

All the Best

Luke

Got links to the reviews? Hard to discuss in the abstract.

But generally, if it is intended to change observable behavior of the LLVM program (if you have to modify a lit test, that’s not NFC, if one could craft a test (that doesn’t invoke UB, though that shouldn’t be possible through the command line interface, etc) that would need to be modified when the patch is committed - then it’s not NFC).

But I think it’s important that NFC is about intent, not about the reality of what the patch ends up doing - so we can’t judge an NFC patch by whether it causes a regression later - the NFC marker is there to say “I don’t intend this to cause any observable change, if you observe any change, that’s a bug” in the same way any patch description is a statement of intent and can’t be more than that.

That’s my litmus test: I see NFC is an indication that no test changes and none are expected to be provided. The functional behavior of the compiler is unchanged. I use NFC on API changes and refactoring when it fits this description.

We could improve the doc maybe?

Got links to the reviews? Hard to discuss in the abstract.

But generally, if it is intended to change observable behavior of the LLVM
program (if you have to modify a lit test, that's not NFC, if one could
craft a test (that doesn't invoke UB, though that shouldn't be possible
through the command line interface, etc) that would need to be modified
when the patch is committed - then it's not NFC).

That's my litmus test: I see NFC is an indication that no test changes and
none are expected to be provided. The functional behavior of the compiler
is unchanged. I use NFC on API changes and refactoring when it fits this
description.

We could improve the doc maybe?

I consider anything modifying an external function/variable (e.g. adding a
parameter, changing the state of a default argument, deleting an unused
function, etc) a functional change.

I consider that refactoring inside a function can be NFC, e.g.

* add/delete/remove local variables
* simplify function-local code

Pure test updates can be seen NFC but I usually tag such commits as `[test]`
to make it clear no code is touched.

It may be less clear whether removing an internal linkage function /
extracting some logic into an internal linkage function is a function
change. Emmm I think that can be NFC.

Sometimes people use the term "NFCI" (no functional change intended).
I thought "intended" means that: the author is not 100% sure that no
functional change is caused (for some refactoring it is sometimes
difficult to guarantee without good test coverage)
but seems that many use "NFCI" to refer to obviously NFC things.

Yeah, I expect NFC changes to be purely internal implementation
details. Changing the public interface (even to add a feature)
isn’t NFC; neither is anything that can affect output.

There are a few places where LLVM does track global state that we don’t
normally consider “output”. I think a change that created fewer
llvm::ConstantExpr’s by, say, not generating a bitcast that only gets
stripped/replaced a few cycles later would still count as NFC to me,
since we don’t really consider the set of unused ConstantExprs to
be output. But those are clarifications of the general rule, not
exceptions.

John.

Got links to the reviews? Hard to discuss in the abstract.

I was more intent on general discussion rather than singling people out, but
here these are things that have caused me to have to modify our downstream
users due to crashes or failed builds this week (these patches were not not
committed in the last week, but I'm a bit behind):
  1. [bc7d15c61da7](⚙ D101713 [OpaquePtr] Use ArgListEntry indirect types more in ISel lowering) changed the format of the
    IR and causes crashes during inlining. Yesterday tanother user commented on
    this one on Phab saying it broke their downstream.
  2. [c83cd8feef7e](Login) changed the order of
    parameters for a public function
  3. [738abfdbea2]([NFC] Remove checking pointee type for byval/preallocated type · llvm/llvm-project@738abfd · GitHub)
    Apparent followup to (1). No code review linked. Reverted this morning.

But generally, if it is intended to change observable behavior of the LLVM
program (if you have to modify a lit test, that's not NFC, if one could craft
a test (that doesn't invoke UB, though that shouldn't be possible through the
command line interface, etc) that would need to be modified when the patch is
committed - then it's not NFC).

I appreciate that downstream consumers aren't afforded the same expectations of
API and ABI stability as an old-fashioned C library users, but it's not just the
function of the programs like `opt` that is the set of all things functional:
the API and the ABI are too.

But I think it's important that NFC is about intent, not about the reality of
what the patch ends up doing - so we can't judge an NFC patch by whether it
causes a regression later - the NFC marker is there to say "I don't intend
this to cause any observable change, if you observe any change, that's a bug"
in the same way any patch description is a statement of intent and can't be
more than that.

Sure. Nobody's perfect, and I'm not saying that *in-general* it's possible to
prove that *any* change - however small - doesn't affect the observable
behaviour of the program. But if we're just going to wave the NFC flag
willy-nilly it completely loses its meaning.

Since Jun 1st, there were ~130 commits in the llvm/ directory marked with
/\bNFC\b/. Many of them are simply formatting or refactoring of the bodies of
functions to clarify the codebase - and this seems appropriate usage to me. Some
of them, however, change the observable program semantics: one removes a command
line flag (breaks people's use of the llvm "program"); and one changes a public
function return type. Both these examples have the potential to break someone's
use of llvm. Yet another is a revert, which suggests to me that the use of NFC
in the original commit might have been less than judicious.

If I'm debugging a new crash, build failure, or codegen change in my downstream,
it'd be nice if I could ignore messages marked "NFC" when trying to find the
commit that introduced the new behaviour. That's *my* benchmark for
non-functional.

All the Best

Luke

Yeah, I expect NFC changes to be purely internal implementation
details. Changing the public interface (even to add a feature)
isn’t NFC; neither is anything that can affect output.

There are a few places where LLVM does track global state that we
don’t
normally consider “output”. I think a change that created fewer
llvm::ConstantExpr’s by, say, not generating a bitcast that only gets
stripped/replaced a few cycles later would still count as NFC to me,
since we don’t really consider the set of unused ConstantExprs to
be output. But those are clarifications of the general rule, not
exceptions.

This seems like a sensible balance to me.

> Got links to the reviews? Hard to discuss in the abstract.
>
> But generally, if it is intended to change observable behavior of the LLVM
> program (if you have to modify a lit test, that's not NFC, if one could
> craft a test (that doesn't invoke UB, though that shouldn't be possible
> through the command line interface, etc) that would need to be modified
> when the patch is committed - then it's not NFC).
>

That's my litmus test: I see NFC is an indication that no test changes and
none are expected to be provided. The functional behavior of the compiler is
unchanged. I use NFC on API changes and refactoring when it fits this
description.

I think this is a bit liberal as it ignores API users - unless I'm
misunderstanding your statement about what you mean by "API changes".

We could improve the doc maybe?

I'm happy to do this legwork but will hold off until something of a consensus
emerges.

All the Best

Luke

For what it’s worth, my understanding was that NFC can also include API changes, as long as they are, well, non-functional. In LLVM pretty much everything is part of the public API, so non-functional refactoring will often touch the API.

To give an example, moving some helper from Transform/Utils to Analysis would be a classical NFC change to me, even if it obviously affects the public API. Another classical NFC change is to rename a function in line with the new naming guidelines. The NFC-ness of that change should not depend on whether that function happens to be exported or not.

Regards,

Nikita

Sometimes people use the term "NFCI" (no functional change intended).
I thought "intended" means that: the author is not 100% sure that no
functional change is caused (for some refactoring it is sometimes
difficult to guarantee without good test coverage)
but seems that many use "NFCI" to refer to obviously NFC things.

This is my understanding too. Even said - I still don't think this can ever
apply to function signature changes as you suggest:

I consider anything modifying an external function/variable (e.g. adding a
parameter, changing the state of a default argument, deleting an unused
function, etc) a functional change.

All the Best

Luke

Yes I am ignoring API users, I am on the same line as Nikita here.
We don’t have stable APIs (other than the C one), so I for example I may change an API that was taking 3 bools to take now a struct parameter wrapping the 3 bools instead. I’ll tag it NFC.

On the same line as my comment above, if I review a patch without any tests, I will ask if it NFC.

Best,

Outsider comment here: I would consider an API change as NFC only if the change causes code relying on the previous version of the API to fail compile. At that point it is obvious that the API has changed and a fix is needed, although it may not be obvious what that fix needs to be.

On some occasions I have dealt with API changes where compiling my old code with the new API results in a correctly-compiling program. However, the resulting application fails to run correctly. This issue is much harder to track down.

Got links to the reviews? Hard to discuss in the abstract.

I was more intent on general discussion rather than singling people out, but
here these are things that have caused me to have to modify our downstream
users due to crashes or failed builds this week (these patches were not not
committed in the last week, but I’m a bit behind):

  1. bc7d15c61da7 changed the format of the
    IR and causes crashes during inlining. Yesterday tanother user commented on
    this one on Phab saying it broke their downstream.

Fair point there - if a patch touches both production and test code that’s a pretty good sign it’s not NFC. I should’ve caught that in review.

  1. c83cd8feef7e changed the order of
    parameters for a public function

(slight typo in the URL, here’s a good one: https://reviews.llvm.org/D99182 )

LLVM doesn’t really have a concept of a “public” API - as Nikita and Mehdi have touched on - this sort of change is the bread and butter of NFC changes in LLVM - refactoring APIs (renaming, adding/removing parameters, etc) and simultaneously updating all callers in such a way that no observable change in the LLVM command line utilities can be observed.

  1. 738abfdbea2
    Apparent followup to (1). No code review linked. Reverted this morning.

Looks like a reasonable “I thought this was NFC/that the invariant was already enforced elsewhere so the checking would be redundant here” and it didn’t turn out to be true.

This change looks in line with what Arthur should be committing with post-commit review. The opaque pointers work is going to be a lot of cleanup like this and I’m happy to review it post-commit in many cases like these small targeted changes - not sure if there’s more scope for testing that might’ve identified the reason it broke things in advance of committing the change.

Perhaps it is NFC, if (1) stuck (regardless of whether (1) was classified as NFC), I haven’t looked closely at whether it was reverted for the patch itself, or because it needed to be backed out if (1) was backed out.

But generally, if it is intended to change observable behavior of the LLVM
program (if you have to modify a lit test, that’s not NFC, if one could craft
a test (that doesn’t invoke UB, though that shouldn’t be possible through the
command line interface, etc) that would need to be modified when the patch is
committed - then it’s not NFC).

I appreciate that downstream consumers aren’t afforded the same expectations of
API and ABI stability as an old-fashioned C library users, but it’s not just the
function of the programs like opt that is the set of all things functional:
the API and the ABI are too.

I don’t think that’s how the LLVM project should/is going to classify “functional change” - in part because there isn’t a clear “public” API boundary - there are wide interfaces across the whole project that external users can call into.

We could introduce a separate term but I think most NFC patches would use that term instead, so it probably wouldn’t amount to much real change - we’d end up using that new term ubiquitously instead.

But I think it’s important that NFC is about intent, not about the reality of
what the patch ends up doing - so we can’t judge an NFC patch by whether it
causes a regression later - the NFC marker is there to say “I don’t intend
this to cause any observable change, if you observe any change, that’s a bug”
in the same way any patch description is a statement of intent and can’t be
more than that.
Sure. Nobody’s perfect, and I’m not saying that in-general it’s possible to
prove that any change - however small - doesn’t affect the observable
behaviour of the program. But if we’re just going to wave the NFC flag
willy-nilly it completely loses its meaning.

I’m not proposing waving it around will-nilly, I think I (& others) have described a fairly consistent understanding of the term.

Since Jun 1st, there were ~130 commits in the llvm/ directory marked with
/\bNFC\b/. Many of them are simply formatting or refactoring of the bodies of
functions to clarify the codebase - and this seems appropriate usage to me. Some
of them, however, change the observable program semantics: one removes a command
line flag (breaks people’s use of the llvm “program”);

Sure, I probably would agree that shouldn’t be NFC.

and one changes a public
function return type.

I’m generally OK with that being marked NFC - as a few folks have said on this thread, cross-library API refactorings are generally understood to be “NFC” in the way the project uses the term.

Both these examples have the potential to break someone’s
use of llvm. Yet another is a revert, which suggests to me that the use of NFC
in the original commit might have been less than judicious.

That the patch was reverted doesn’t necessarily mean the original commit was a problem - looks like it was reverted because a preceeding patch was reverted that meant the follow on patch needed to be reverted too, without that follow on patch necessarily being to blame.

If I’m debugging a new crash, build failure, or codegen change in my downstream,

Build failures (if you mean like Clang no longer compiling some code it did before - not “a previously compiling use of LLVM libraries now doesn’t compile anymore”) and codegen changes (if you mean LLVM produces different IR/machine code for the same source code - not LLVM itself builds to a different binary file) shouldn’t be marked NFC - that’s sort of the core of the function of LLVM and is the definition of a functional change.

Crashes - they happen.

I would consider an even more restricted subset: NFC changes would be
trivial proven to be just that. That means non-trivial algorithmic
changes are certainly not NFC. My point is that the tag should be
applied with care only and if there is any doubt, it should not be used
at all.

Joerg

I disagree pretty strongly there - intent is important and all any patch comment can describe.

If that change was adopted, I’d want another flag for the “I intend this not to change the observable behavior, but it’s not trivially proven” - especially when reviewing a patch that is missing test coverage. If someone hasn’t made a claim that this doesn’t change behavior, then I expect that the inverse is true (it does change behavior) and a test should be present. And once we have that flag, I’m not sure the difference between “I intend this to not change behavior, but it passes some threshold of triviality that makes it not ‘guaranteed’” and things below that threshold is sufficiently valuable to encode in the commit message (& haggle over which things are over or under that threshold).

  • Dave

+1

Philip

Hi Luke,

I would like to add a thought in another direction. We could reduce some of the pain with two flavors of continuous integration:

  1. We could be running some automatic integration testing with downstream users: Have some __public__ CI machine build the heads of LLVM and a downstream project (e.g. Rust, Tensorflow) and see if the builds still pass.

This would make it easier to identify the breakages and provide feedback to a) the person implementing the change and b) the downstream users who might need to adapt to that change. At least a couple of Rust and TensorFlow folks would be interested in that. However that only works for open source usages of LLVM.

Augie and I set up a __prototype for a LLVM + Rust CI build__ [1] to see what that would look like. This would certainly need more discussion and work for production use.

  1. Not really addressing your ABI/API use case: We could try to move towards __100% pre-merge testing__ (i.e. building every change before merging it to the main branch). I chased down a couple of “NFCs” that broke the build or some test. That could have been prevented with pre-merge testing. This is not trivial to implement and would require a workflow change.

This approach would not solve the tagging question, but we could reduce some of the pains around debugging broken builds.

[1] https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds?branch=master

I am against such thing for the following reasons:
- LLVM developers cannot see whether their commit or a downstream
commit caused a failure.
- It gives those selected projects privileged access to the LLVM
development direction. Suddenly downstream problems become blockers
(e.g. downstream code has undefined behaviour and we cannot improve
the optimizer until downstream is fixed).
- It is also unnecessary. We already have the llvm-test-suite.
Downstream projects can add their code here as well.

Downstream CI maintainers can already report problems that they
identified to be caused by Clang to the LLVM project, like the
Chromium and Fuchsia teams do and their CI runs are also accessible
via public webpages
(chromium.clang).
However, the burden of identifying the problem is theirs.

Michael

I agree with most of the discussion in the thread:

“NFC” is intended to talk about the behavior of the compiler, not its API. I agree with Mehdi that it is intended (among other things) to be an explanation of why there are no new tests or changes to existing tests.

I don’t think that extending it to mean “no C++ API were made”. In fact, one of the major intentions of “NFC” is to encourage people to do land large changes as a series of NFC refactorings.

That said, I agree with you that changes to the IR itself are not NFC. This is an observable change to the compiler behavior and should require test updates.

-Chris

+1