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. 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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210617/f2731c42/attachment.html>
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 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.