<div dir="ltr"><div dir="ltr">On Fri, Jun 18, 2021 at 4:00 AM Luke Drummond <<a href="mailto:luke.drummond@codeplay.com">luke.drummond@codeplay.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu Jun 17, 2021 at 6:15 PM BST, David Blaikie wrote:<br>
> Got links to the reviews? Hard to discuss in the abstract.<br>
<br>
I was more intent on general discussion rather than singling people out, but<br>
here these are things that have caused me to have to modify our downstream<br>
users due to crashes or failed builds this week (these patches were not not<br>
committed in the last week, but I'm a bit behind):<br>
  1. [bc7d15c61da7](<a href="https://reviews.llvm.org/D101713" rel="noreferrer" target="_blank">https://reviews.llvm.org/D101713</a>) changed the format of the<br>
    IR and causes crashes during inlining. Yesterday tanother user commented on<br>
    this one on Phab saying it broke their downstream.<br></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
  2. [c83cd8feef7e](<a href="https://reviews.llvm.org/D9918l2" rel="noreferrer" target="_blank">https://reviews.llvm.org/D9918l2</a>) changed the order of<br>
    parameters for a public function<br></blockquote><div><br>(slight typo in the URL, here's a good one: <a href="https://reviews.llvm.org/D99182">https://reviews.llvm.org/D99182</a> )<br><br>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
  3. [738abfdbea2](<a href="https://github.com/llvm/llvm-project/commit/738abfdbea2" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/738abfdbea2</a>)<br>
    Apparent followup to (1). No code review linked. Reverted this morning.<br></blockquote><div><br></div><div>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.<br><br>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.<br><br>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> But generally, if it is intended to change observable behavior of the LLVM<br>
> program (if you have to modify a lit test, that's not NFC, if one could craft<br>
> a test (that doesn't invoke UB, though that shouldn't be possible through the<br>
> command line interface, etc) that would need to be modified when the patch is<br>
> committed - then it's not NFC).<br>
<br>
I appreciate that downstream consumers aren't afforded the same expectations of<br>
API and ABI stability as an old-fashioned C library users, but it's not just the<br>
function of the programs like `opt` that is the set of all things functional:<br>
the API and the ABI are too.<br></blockquote><div><br>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.<br><br>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.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> But I think it's important that NFC is about intent, not about the reality of<br>
> what the patch ends up doing - so we can't judge an NFC patch by whether it<br>
> causes a regression later - the NFC marker is there to say "I don't intend<br>
> this to cause any observable change, if you observe any change, that's a bug"<br>
> in the same way any patch description is a statement of intent and can't be<br>
> more than that.<br>
Sure. Nobody's perfect, and I'm not saying that *in-general* it's possible to<br>
prove that *any* change - however small - doesn't affect the observable<br>
behaviour of the program. But if we're just going to wave the NFC flag<br>
willy-nilly it completely loses its meaning.<br></blockquote><div><br></div><div>I'm not proposing waving it around will-nilly, I think I (& others) have described a fairly consistent understanding of the term.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Since Jun 1st, there were ~130 commits in the llvm/ directory marked with<br>
/\bNFC\b/. Many of them are simply formatting or refactoring of the bodies of<br>
functions to clarify the codebase - and this seems appropriate usage to me. Some<br>
of them, however, change the observable program semantics: one removes a command<br>
line flag (breaks people's use of the llvm "program"); </blockquote><div><br></div><div>Sure, I probably would agree that shouldn't be NFC.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">and one changes a public<br>
function return type. </blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Both these examples have the potential to break someone's<br>
use of llvm. Yet another is a revert, which suggests to me that the use of NFC<br>
in the original commit might have been less than judicious.<br></blockquote><div><br>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. <br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
If I'm debugging a new crash, build failure, or codegen change in my downstream,<br></blockquote><div><br>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.<br><br>Crashes - they happen.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
it'd be nice if I could ignore messages marked "NFC" when trying to find the<br>
commit that introduced the new behaviour. That's *my* benchmark for<br>
non-functional.<br>
<br>
All the Best<br>
<br>
Luke<br>
-- <br>
Codeplay Software Ltd.<br>
Company registered in England and Wales, number: 04567874<br>
Registered office: Regent House, 316 Beulah Hill, London, SE19 3HF<br>
</blockquote></div></div>