On Thu, Jun 17, 2021 at 10:15 AM David Blaikie via llvm-dev < llvm-dev at 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? -- Mehdi> > 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. > > On Thu, Jun 17, 2021 at 10:11 AM Luke Drummond via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> 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 >> -- >> 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-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210617/e4732df5/attachment.html>
On 2021-06-17, Mehdi AMINI via llvm-dev wrote:>On Thu, Jun 17, 2021 at 10:15 AM David Blaikie via llvm-dev < >llvm-dev at 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.> >> >> 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. >> >> On Thu, Jun 17, 2021 at 10:11 AM Luke Drummond via llvm-dev < >> llvm-dev at lists.llvm.org> wrote: >> >>> 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 >>> -- >>> 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-dev at lists.llvm.org >>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>_______________________________________________ >LLVM Developers mailing list >llvm-dev at lists.llvm.org >https://lists.llvm.org/cgi-bin/mailman/listinfo/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. 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. 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
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-dev at 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. 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