[llvm-dev] "[NFC]" Abuse
Luke Drummond via llvm-dev
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
--
Codeplay Software Ltd.
Company registered in England and Wales, number: 04567874
Registered office: Regent House, 316 Beulah Hill, London, SE19 3HF
_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
David Blaikie via llvm-dev
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.
Mehdi AMINI via llvm-dev
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).
Fangrui Song via llvm-dev
>On Thu, Jun 17, 2021 at 10:15 AM David Blaikie via llvm-dev <
>llvm...@lists.llvm.org> wrote:
>
>> 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.
John McCall via llvm-dev
On 17 Jun 2021, at 13:15, David Blaikie via llvm-dev wrote:
> 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.
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.
Luke Drummond via llvm-dev
> 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](https://reviews.llvm.org/D101713) 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](https://reviews.llvm.org/D9918l2) changed the order of
parameters for a public function
3. [738abfdbea2](https://github.com/llvm/llvm-project/commit/738abfdbea2)
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.
Luke Drummond via llvm-dev
> 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.
--
Codeplay Software Ltd.
Company registered in England and Wales, number: 04567874
Registered office: Regent House, 316 Beulah Hill, London, SE19 3HF
Luke Drummond via llvm-dev
> On Thu, Jun 17, 2021 at 10:15 AM David Blaikie via llvm-dev <
> llvm...@lists.llvm.org> wrote:
>
> > 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.
Nikita Popov via llvm-dev
On Thu Jun 17, 2021 at 7:15 PM BST, Mehdi AMINI wrote:
> On Thu, Jun 17, 2021 at 10:15 AM David Blaikie via llvm-dev <
> llvm...@lists.llvm.org> wrote:
>
> > 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".
Luke Drummond via llvm-dev
> 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
--
Codeplay Software Ltd.
Company registered in England and Wales, number: 04567874
Registered office: Regent House, 316 Beulah Hill, London, SE19 3HF
Mehdi AMINI via llvm-dev
On Thu Jun 17, 2021 at 7:15 PM BST, Mehdi AMINI wrote:
> On Thu, Jun 17, 2021 at 10:15 AM David Blaikie via llvm-dev <
> llvm...@lists.llvm.org> wrote:
>
> > 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".
David Jones via llvm-dev
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,—Mehdi
> 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
--
Codeplay Software Ltd.
Company registered in England and Wales, number: 04567874
Registered office: Regent House, 316 Beulah Hill, London, SE19 3HF
_______________________________________________
David Blaikie via llvm-dev
On Thu Jun 17, 2021 at 6:15 PM BST, David Blaikie wrote:
> 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](https://reviews.llvm.org/D101713) 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.
2. [c83cd8feef7e](https://reviews.llvm.org/D9918l2) 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.
3. [738abfdbea2](https://github.com/llvm/llvm-project/commit/738abfdbea2)
Apparent followup to (1). No code review linked. Reverted this morning.
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.
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.
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.
Joerg Sonnenberger via llvm-dev
> 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.
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
David Blaikie via llvm-dev
On Thu, Jun 17, 2021 at 03:13:16PM -0400, John McCall via llvm-dev wrote:
> 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.
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.
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
Philip Reames via llvm-dev
+1
Philip
Christian Kühnel via llvm-dev
Michael Kruse via llvm-dev
llvm-dev <llvm...@lists.llvm.org>:
> 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.
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
(https://ci.chromium.org/p/chromium/g/chromium.clang/console).
However, the burden of identifying the problem is theirs.
Michael
Chris Lattner via llvm-dev
>
> 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.
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
Hubert Tong via llvm-dev
On Fri, Jun 18, 2021 at 4:07 AM Luke Drummond <luke.d...@codeplay.com> wrote:On Thu Jun 17, 2021 at 7:15 PM BST, Mehdi AMINI wrote:
> On Thu, Jun 17, 2021 at 10:15 AM David Blaikie via llvm-dev <
> llvm...@lists.llvm.org> wrote:
>
> > 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".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.
Luke Drummond via llvm-dev
Thanks for all the responses.
Since there have been quite a few separate alignments one way or another I'll
try and summarize everything mentioned in one mail, being fairly liberal with
the quoting and paraphrasing. Ed: please forgive.
Basically it seems there are two distinct groups:
Those who don't consider API changes as "functional" consider LLVM's in-tree
command line tools as the "function", and consider changes to the API fair game
for the NFC tag.
I argued that API changes are, by definition, not NFC. It was fairly strongly
rejected by several:
David Blaikie:
> 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.
Nikita Popov:
> 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.
Mehdi Amini:
> 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.
Chris Lattner mostly agrees with the above:
> "NFC" is intended to talk about the behavior of the compiler, not its API
and
> [...] "NFC" *encourage people* to land large changes as a series of NFC
> refactorings.
I think the final point is pretty telling about the use of [NFC] among llvm
developers historically.
Historically I can see the attraction of minimizing the cognitive barrier to
addressing poorly factored code, but perhaps now it's an opportunity to
rubber-stamp your commits to avoid the slow review process? When we have a
mature system, fairly robust test-suite, code-review, and CI, I don't think the
minor benefit of softening the armour is necessary when the sorts of changes
that actually do require changing public APIs are almost never done by newcomers
[citation needed].
To continue doing this when downstreams have to refactor their code every couple
of days, and - crucially - find the commit to blame is frustrating.
Then there are those more conservative devs whose wishes align more with my own.
David Jones:
> 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.
I think this distills my argument. Sometimes due to promotions things like
this get past the compiler with no change of the user:
- void maybeDoThing(unsigned Count, bool AreYouSure);
+ int maybeDoThing(bool AreYouSure, unsigned count);
...and then do something pretty nasty when you least expect it. If this commit
is marked NFC, the committer has actively misdirected anyone who doesn't
understand LLVM's unusual use of "NFC", so the barrier to using LLVM becomes
higher again. Do this enough times and using LLVM's API becomes a cruel and
unusual punishment ;)
John McCall:
> 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.
Joerg Sonnenberger:
> I would consider an even more restricted subset: NFC changes would be
trivial proven to be just that
David Blaikie in response to Joerg and John
> 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).
I'd say that David is already describing a tag in popular use: "NFCI" is what's
conventionally used outside LLVM (and even within LLVM by others) to tag changes
that match exactly that description.
David also sort of foreshadowed this opinion in another message:
> 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.
Fangrui Song mentioned the following in a separate message which - to me -
largely aligns with David's intent:
> 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.
I think that in the face of an extant "NFCI" tag, these 3 opinions actually
*are* compatible with each other, but whether those that advocate for "NFC"
where others have described "NFCI" are aware of the existence of this tag is
unclear to me.
Another point was raised about downstreams and pre-commit testing by Christian
Kühnel. Christian suggested integration of upstream LLVM and a number of popular
downstreams' CI systems:
> 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.
I think this is noble but misses the essence of my original point: It's not that
LLVM changes *per-se* that's troublesome as a downstream. It's that LLVM
changes but the committer that made the change has said that it's not changed.
LLVM *should* continue to change, but I still think the NFC tag is incorrect
with respect to API changes, and any gentle push to make life easier to
downstreams would be something I'm sure would be appreciated. Applying "[NFC]"
to silent breakages goes somewhat beyond "we provide no API stability" (which
is clear and well understood by the community) to "this is active misdirection
that makes your life harder as a maintainer".
In summary, I think there's still consensus to be made, and if us loud voices
have equal standing, there's little majority, so I don't think I can formalize
this for the documentation yet.
Thanks for all the input so far. Apologies if I missed anyone.
More voices welcome.
David Blaikie via llvm-dev
<llvm...@lists.llvm.org> wrote:
>
> Hi all
>
> Thanks for all the responses.
>
> Since there have been quite a few separate alignments one way or another I'll
> try and summarize everything mentioned in one mail, being fairly liberal with
> the quoting and paraphrasing. Ed: please forgive.
Thanks for the summary!
I don't think it does that - to me the only difference with "NFC" when
reviewing a patch (pre or post-commit) is "Is this intended to be
testable/tested". If it says NFC I expect to see no tests updated and
review the code change to ensure/verify that it shouldn't/couldn't be
tested (in addition to whatever change-specific review to do).
> When we have a
> mature system, fairly robust test-suite, code-review, and CI, I don't think the
> minor benefit of softening the armour is necessary when the sorts of changes
> that actually do require changing public APIs are almost never done by newcomers
> [citation needed].
I lost track of this bit a bit. We do often expect newcomers to do API
changes - one of the perennial "easy things for a newcomer to do" is
to change APIs that take other string types to take StringRef for
example - which is generally an NFC API change.
(aside: folks keep saying "public API" - LLVM has no clear distinction
of public (if we're using that in the sense of "outside the LLVM
project") and private API - there's some library internal APIs (those
where the header is in the lib directory rather than the include
directory) but everything else is "inter-library API" which is about
as "public" as it gets, I guess - if other parts of LLVM can use it,
so can people using LLVM's libraries from outside)
> To continue doing this when downstreams have to refactor their code every couple
> of days, and - crucially - find the commit to blame is frustrating.
>
> Then there are those more conservative devs whose wishes align more with my own.
>
> David Jones:
> > 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.
Generally we should be somewhat careful of such a refactor because we
might miss a call site inside LLVM too - but I wouldn't rule it out.
It's not hard inside LLVM to search for all the call sites and update
them. I think no matter the words used in the commit message I think
that's always going to be a reality in the LLVM project (that there
might be silently incompatible API changes).
> I think this distills my argument. Sometimes due to promotions things like
> this get past the compiler with no change of the user:
>
> - void maybeDoThing(unsigned Count, bool AreYouSure);
> + int maybeDoThing(bool AreYouSure, unsigned count);
>
> ...and then do something pretty nasty when you least expect it. If this commit
> is marked NFC, the committer has actively misdirected anyone who doesn't
> understand LLVM's unusual use of "NFC",
Do you have references on broader/other/"usual" uses of the term NFC and NFCI?
At least my attempts to google the terms seem to only turn up LLVM and
LLVM-adjacent (eg: Swift) results.
> so the barrier to using LLVM becomes
> higher again. Do this enough times and using LLVM's API becomes a cruel and
> unusual punishment ;)
>
> John McCall:
> > 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.
This might be misquoted/out of context. It looks to me like John was
agreeing with my description - John's concept of "purely internal
implementation details" may include what you are describing as "Public
API" - the public interface John is probably talking about (I think,
given he opens with "yeah" in response to my description) is LLVM IR,
bitcode, and command line tools - not APIs LLVM uses internally to
implement that functionality.
Conflating the "there can be silent API breakage that causes semantic
changes without build breaks" and "there are changes tagged NFC that
change functionality" (due to developer mistakes/bugs) and "there are
changes tagged NFC that change APIs" together is probably confusing
things a bit. For instance I think the first of those is relatively
unavoidable - and how something's tagged isn't going to change that.
I don't think NFCI v NFC (I don't mind people choosing one or the
other, but I don't think as a community we should try to split hairs
and either say "you can't say NFC, you have to say NFCI always" or
"here's the level of complexity over which we aren't comfortable
saying it's for sure, so it's only 'intent'") really helps - all patch
descriptions are statements of intent, not proven fact - we all write
bugs, if we have to add defensive wording for the possibility of bugs
in any commit message, I don't think that's going to improve
clarity/communication. And that distinction still wouldn't address the
desire for API changes to not be flagged NFC[I] - which, again, I
think is a bad thing, annotating changes that don't/shouldn't be
tested is a core property of the use of the NFC[I] flag.
- Dave
John McCall via llvm-dev
> On Wed, Jun 23, 2021 at 10:34 AM Luke Drummond via llvm-dev
> <llvm...@lists.llvm.org> wrote:
>> so the barrier to using LLVM becomes
>> higher again. Do this enough times and using LLVM's API becomes a
>> cruel and
>> unusual punishment ;)
>>
>> John McCall:
>>> 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.
>
> This might be misquoted/out of context. It looks to me like John was
> agreeing with my description - John's concept of "purely internal
> implementation details" may include what you are describing as "Public
> API" - the public interface John is probably talking about (I think,
> given he opens with "yeah" in response to my description) is LLVM IR,
> bitcode, and command line tools - not APIs LLVM uses internally to
> implement that functionality.
I may have misunderstood your point. I consider changes to the public
interfaces of a system (defined as interfaces outside a library) to
generally not be NFC. This is C++, so that isn’t as simple as “no
changes to headers”; it means, well, roughly the sorts of things that
you would describe in a C++ concept: the names and signatures of
functions, the operations supported by types, and so on. Taking three
bool arguments and making them a struct is not NFC. Adding a new
non-private method to a class is not NFC.
We should absolutely encourage refactors that improve the quality
of both the implementation and its interfaces, but I don’t treat
NFC as a tool to that end, and I’m surprised to hear that other
maintainers do. It feels like you’re using NFC to mean almost
“questionable” and then looking for any excuse not to label a patch
NFC.
John.
David Blaikie via llvm-dev
Ah, OK - yeah, that's not my usage/understanding of it.
Here are a few examples that seem to be inconsistent with that usage,
made or reviewed by fairly core/frequent LLVM contributors:
https://github.com/llvm/llvm-project/commit/fdaf304e0d984c9f919c6b6b2b108d0d31cbea87
https://github.com/llvm/llvm-project/commit/56709b869570f7825d335d633bc829511980c253
https://github.com/llvm/llvm-project/commit/dd1b121c99de6bd7186e23e2bf34edb02db7c076
(local to a tool, so probably not a perfect example)
https://github.com/llvm/llvm-project/commit/9a23e673caebdd54d8cc285fcad78f18fa2e919a
(new (moved from lib/ into include/) class in
llvm/include/llvm/Transforms/IPO/Attributor.h )
https://github.com/llvm/llvm-project/commit/cfb96d845a684a5c567823dbe2aa4392937ee979
(change return types and parameters from pointer to reference in
lldb/include/lldb/Breakpoint)
https://github.com/llvm/llvm-project/commit/7629b2a09c169bfd7f7295deb3678f3fa7755eee
(new functions (moved from lib/ into include/) in
llvm/include/llvm/Analysis/LoopInfo.h )
https://github.com/llvm/llvm-project/commit/84ab315574099dcac8f9fb89fe8558f8ccfbce5f
(new function (moved from lib/ into include/) in Sema)
> We should absolutely encourage refactors that improve the quality
> of both the implementation and its interfaces, but I don’t treat
> NFC as a tool to that end, and I’m surprised to hear that other
> maintainers do. It feels like you’re using NFC to mean almost
> “questionable” and then looking for any excuse not to label a patch
> NFC.
"you" in this case being Luke, I take it?
- Dave
Mehdi AMINI via llvm-dev
On 23 Jun 2021, at 14:57, David Blaikie wrote:
> On Wed, Jun 23, 2021 at 10:34 AM Luke Drummond via llvm-dev
> <llvm...@lists.llvm.org> wrote:
>> so the barrier to using LLVM becomes
>> higher again. Do this enough times and using LLVM's API becomes a
>> cruel and
>> unusual punishment ;)
>>
>> John McCall:
>>> 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.
>
> This might be misquoted/out of context. It looks to me like John was
> agreeing with my description - John's concept of "purely internal
> implementation details" may include what you are describing as "Public
> API" - the public interface John is probably talking about (I think,
> given he opens with "yeah" in response to my description) is LLVM IR,
> bitcode, and command line tools - not APIs LLVM uses internally to
> implement that functionality.
I may have misunderstood your point. I consider changes to the public
interfaces of a system (defined as interfaces outside a library) to
generally not be NFC.
John McCall via llvm-dev
This one is interesting because it’s basically just an improvement to
the static typing in a way that should be harmless.
I can see why someone could reasonably argue that this is NFC because
it’s just adding some constants in a reusable place, and that place
happens to be public.
https://github.com/llvm/llvm-project/commit/dd1b121c99de6bd7186e23e2bf34edb02db7c076
(local to a tool, so probably not a perfect example)
https://github.com/llvm/llvm-project/commit/9a23e673caebdd54d8cc285fcad78f18fa2e919a
(new (moved from lib/ into include/) class in
llvm/include/llvm/Transforms/IPO/Attributor.h )
The “promoting existing code to public” case, sure, I can see.
https://github.com/llvm/llvm-project/commit/cfb96d845a684a5c567823dbe2aa4392937ee979
(change return types and parameters from pointer to reference in
lldb/include/lldb/Breakpoint)
This is definitely beyond what I would call NFC. It’s a good
change, but I mean, so would be not having multiple pass managers.
https://github.com/llvm/llvm-project/commit/7629b2a09c169bfd7f7295deb3678f3fa7755eee
(new functions (moved from lib/ into include/) in
llvm/include/llvm/Analysis/LoopInfo.h )
https://github.com/llvm/llvm-project/commit/84ab315574099dcac8f9fb89fe8558f8ccfbce5f
(new function (moved from lib/ into include/) in Sema)
We should absolutely encourage refactors that improve the quality
of both the implementation and its interfaces, but I don’t treat
NFC as a tool to that end, and I’m surprised to hear that other
maintainers do. It feels like you’re using NFC to mean almost
“questionable” and then looking for any excuse not to label a patch
NFC."you" in this case being Luke, I take it?
Ah, no, I meant you, but I wrote it out completely wrong: I meant
that it feels like the people using NFC this broadly almost mean
that a patch not being NFC makes it especially questionable
and then are looking for nearly any reason to mark it NFC.
The examples you’ve given are almost all just adding API surface
area, and moreover adding it by just making something public that
wasn’t before. To me, the purpose of NFC is to indicate that a
change is a fairly trivial and should-be-uncontroversial improvement
to the code, suitable for a quick sign-off. So I can see an argument
those kinds of changes being NFC because the original private
interface should’ve been considered and reviewed at least a little
bit when it was added in the first place. But no, I still don’t
think arbitrary changes to an existing API should be thought of as NFC.
That doesn’t mean they shouldn’t happen, it just means reviewers
should be at least slightly more engaged.
John.
Mehdi AMINI via llvm-dev
In summary, I think there's still consensus to be made, and if us loud voices
have equal standing, there's little majority, so I don't think I can formalize
this for the documentation yet.
James Henderson via llvm-dev
Adam HARRIES via llvm-dev
As a fairly new member of the llvm community, I'm interested in what people mean when they say "users". For me, there are two distinct groups that I could consider "users" of the llvm-project: Day-to-day developers who use the compiled tools to build separate software, and compiler/tool developers who integrate or build upon llvm to build a piece of software. From reading through this thread it feels like these groups are sometimes being conflated, which leads to disagreements regarding whether or not a change will affect llvm "users". If we only consider group one to be "users", then clearly any change that does not alter the observable behaviours of the binaries can be considered a NFC. However, if we consider group two to be users as well, then the impacts of any change may be a functional change - and I would personally argue that this is especially true for API changes.
Cheers,
James Henderson via llvm-dev
Adam HARRIES via llvm-dev
> if a downstream developer had a private patch using that local variable
via llvm-dev
The LLVM-as-libraries model means that “users” include downstream folks writing their own tooling that uses the APIs; not just people running the tooling LLVM provides that uses the APIs. These changes are “externally visible” (as opposed to internal refactoring that can affect downstream modifications to the insides of LLVM).
--paulr
David Blaikie via llvm-dev
https://github.com/llvm/llvm-project/commit/dd1b121c99de6bd7186e23e2bf34edb02db7c076
(local to a tool, so probably not a perfect example)
https://github.com/llvm/llvm-project/commit/9a23e673caebdd54d8cc285fcad78f18fa2e919a
(new (moved from lib/ into include/) class in
llvm/include/llvm/Transforms/IPO/Attributor.h )The “promoting existing code to public” case, sure, I can see.
https://github.com/llvm/llvm-project/commit/cfb96d845a684a5c567823dbe2aa4392937ee979
(change return types and parameters from pointer to reference in
lldb/include/lldb/Breakpoint)This is definitely beyond what I would call NFC. It’s a good
change, but I mean, so would be not having multiple pass managers.
https://github.com/llvm/llvm-project/commit/7629b2a09c169bfd7f7295deb3678f3fa7755eee
(new functions (moved from lib/ into include/) in
llvm/include/llvm/Analysis/LoopInfo.h )
https://github.com/llvm/llvm-project/commit/84ab315574099dcac8f9fb89fe8558f8ccfbce5f
(new function (moved from lib/ into include/) in Sema)
We should absolutely encourage refactors that improve the quality
of both the implementation and its interfaces, but I don’t treat
NFC as a tool to that end, and I’m surprised to hear that other
maintainers do. It feels like you’re using NFC to mean almost
“questionable” and then looking for any excuse not to label a patch
NFC."you" in this case being Luke, I take it?
Ah, no, I meant you, but I wrote it out completely wrong: I meant
that it feels like the people using NFC this broadly almost mean
that a patch not being NFC makes it especially questionable
and then are looking for nearly any reason to mark it NFC.
John McCall via llvm-dev
> On Wed, Jun 23, 2021 at 2:57 PM John McCall <rjmc...@apple.com> wrote:
>> Ah, no, I meant you, but I wrote it out completely wrong: I meant
>> that it feels like the people using NFC this broadly almost mean
>> and then are looking for nearly any reason to mark it NFC.
>>
> That's not so much my feeling, I think - NFC versus non-NFC for me is
> mostly an indicator of how I should review the patch. Usually hinging on
> "does this have tests/is this testable".
Okay. Well, we use NFC differently, but because I use it more strictly,
at least I won’t bother you.
John.
David Blaikie via llvm-dev
On 24 Jun 2021, at 13:35, David Blaikie wrote:
> On Wed, Jun 23, 2021 at 2:57 PM John McCall <rjmc...@apple.com> wrote:
>> Ah, no, I meant you, but I wrote it out completely wrong: I meant
>> that it feels like the people using NFC this broadly almost mean
>> that a patch *not* being NFC makes it especially questionable
>> and then are looking for nearly any reason to mark it NFC.
>>
> That's not so much my feeling, I think - NFC versus non-NFC for me is
> mostly an indicator of how I should review the patch. Usually hinging on
> "does this have tests/is this testable".
Okay. Well, we use NFC differently, but because I use it more strictly,
at least I won’t bother you.
I mean, a little - If I end up reading the patch, seeing no tests & wondering why it's not tested & have to understand more about the patch to see it's changing something that doesn't need test coverage.
But not the biggest deal, no.
- Dave