Fāng-ruì Sòng via llvm-dev
2020-Nov-15 21:59 UTC
[llvm-dev] [RFC] Backend for Motorola 6800 series CPU (M68k)
On Sun, Nov 15, 2020 at 1:27 PM John Paul Adrian Glaubitz via llvm-dev <llvm-dev at lists.llvm.org> wrote:> > On 11/15/20 9:33 PM, Simon Pilgrim via llvm-dev wrote: > > As well as the actual patch reviews, has there been official approval that the > > M68k experimental backend can be added to trunk? I guess we need a > > "Backend: M68k" bugzilla component - is there anything else? > > Sounds good. I'll file that bug tomorrow. > > Thanks, > Adrian > > -- > .''`. John Paul Adrian Glaubitz > : :' : Debian Developer - glaubitz at debian.org > `. `' Freie Universitaet Berlin - glaubitz at physik.fu-berlin.de > `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913I've looked at a few newer backends in LLVM and I think the policy is unclear about how an experimental target is approved and reviewed. * BPF https://lists.llvm.org/pipermail/llvm-dev/2014-December/079897.html Tom said "it's up to Chris Lattner to approve new backends" I cannot find discussions about the approval process around the time it was committed (Jan 2015) * ARC https://lists.llvm.org/pipermail/llvm-dev/2017-August/116268.html "but it would be great to have this in tree" may be vaguely counted as an * VE I cannot find related llvm-dev discussions around the time it was committed (Jan 2020) * CSKY (a stub has been added) https://lists.llvm.org/pipermail/llvm-dev/2020-August/144485.html The closest thing to an approval is the sentence "It is always exciting to see new targets!" Also worth noting that some of the initial review patches have just one or no reviewer on the reviewer list (i.e. probably not get enough scrutiny from community members). (In addition, our Phabricator setting makes a patch green if anyone accepts it - this makes it seem like the target is approved while the reviewer probably just intended that "the patch is in a good shape". Sometimes the author committed it in haste after accepted. Sometimes this may be understood as the author might have waited for too long before getting feedback)
Renato Golin via llvm-dev
2020-Nov-15 23:23 UTC
[llvm-dev] [RFC] Backend for Motorola 6800 series CPU (M68k)
On Sun, 15 Nov 2020 at 21:59, Fāng-ruì Sòng via llvm-dev < llvm-dev at lists.llvm.org> wrote:> I've looked at a few newer backends in LLVM and I think the policy is > unclear about how an experimental target is approved and reviewed. >Hi Fang-rui, It's true that there isn't a definite rubber stamp procedure to approve things in LLVM. We have kept it that way for years because the stricter the rules the harder it is to act on (some definition of) common sense. So we have documented what we expect of targets in the developer's policy [1] and support policy [2]. It's not easy to dig information on those approvals because we don't have a formal process, it's just emails on the list. But in the past, and for all of those targets you mention, the "process" is to reach consensus. That's done by having people voicing their support of the target on the list, and no one bringing substantial concerns. I may have missed something, but I have not seen anyone raising any concerns on m68k, so I think we're good on that side. I personally support the m68k target to be in LLVM and I think it would be a great addition to our list of targets. During the patch reviews is when people are most likely to raise concerns, so before that happens, it wouldn't be appropriate for anyone to "approve" the target, or that would create an issue if the code had too many issues with it. So, once all patches are reviewed, all changes are in, all your local tests are green, and a number of people approve them, and there are no more comments, come back to the list (probably in a new thread) and do a final RFC, saying the code is good to go. You could repeat the basics, like who will be the code owner, the community behind, and who will create the infrastructure to test the experimental builds. That's the thread that, if all responses are positive, you may start the merge of the code into master. Also worth noting that some of the initial review patches have just> one or no reviewer on the reviewer list (i.e. probably not get enough > scrutiny from community members). >>From what I saw, Craig, Roman and others were sending good, detailedreviews, so I refrained from comment. Once everyone is happy and there aren't enough reviewers, let us know and we'll start pinging wider in the community.> (In addition, our Phabricator setting makes a patch green if anyone > accepts it - this makes it seem like the target is approved while the > reviewer probably just intended that "the patch is in a good shape". > Sometimes the author committed it in haste after accepted. Sometimes > this may be understood as the author might have waited for too long > before getting feedback) >Right, this doesn't seem to be clear on our review policy [3] (and may need some adjustment). We work with the assumption that people won't unconditionally approve a patch if they don't have confidence or experience in the area to do so. The policy states that it's up to the reviewer to make their intentions known (please wait, ask others, etc). But authors, especially those that already do or will work closely with our community, should always ask themselves what is the right thing to do, and when in doubt, asking around (review, mail, irc, etc) is the best policy. The expectation for bigger changes is to need multiple reviewers and approvals. New targets, policy updates, etc are the ones that need the largest number of approvals from "a wide representation in the community" (which is not well defined, I agree). Also, if you are submitting a patch series, you should not commit any patch until the whole series is approved. This is to avoid half-baked changes going in, only to be reverted back due to some previously unknown big issue with a patch that wasn't reviewed yet. In your case, you may choose to commit your series in stages (pre, llvm, clang) for example, but you'll still need to wait until the whole series is approved, by multiple people, and then strong support is voiced in the final RFC email to the list. This is special for new targets or very large changes.>From what you wrote, I think that's what you were expecting, but thewriting isn't super clear, so I want to make sure there are no confusions: Don't assume silence is approval. When in doubt, ping people. Most of us are super busy and we can easily forget, so we don't take pings as annoying, but as a necessary part of a highly distributed project. Hope that helps. cheers, --renato [1] http://llvm.org/docs/DeveloperPolicy.html#adding-a-new-target [2] http://llvm.org/docs/SupportPolicy.html [3] http://llvm.org/docs/CodeReview.html PS: If after all reviews you're still stuck and no one else is responding to your pings, feel free to contact me directly. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20201115/4e898418/attachment.html>
David Chisnall via llvm-dev
2020-Nov-16 10:30 UTC
[llvm-dev] [RFC] Backend for Motorola 6800 series CPU (M68k)
Hi, To add to what Renato said: On 15/11/2020 21:59, Fāng-ruì Sòng via llvm-dev wrote:> I've looked at a few newer backends in LLVM and I think the policy is > unclear about how an experimental target is approved and reviewed.It's generally good form to tag anyone who commented in the mailing list threads on any patches, especially people who objected. If they're happy in review, it's probably good to go. Generally, the bar for being in-tree is fairly low, the bar to being removed from the experimental-back-ends list is much higher. An experimental back end is not built by default and is not in any of the binary releases. Experimental back ends provide a probation period for the maintainer community. If bugs are fixed regularly, the back end makes good progress, the required infrastructure is maintained, there's engagement from the maintainers in reviewing other patches then it's easy to get the back end promoted to non-experimental. If not, then it will languish in the experimental state for a while and then eventually be removed (this will typically happen much faster if it requires any infrastructure in the generic CodeGen logic to support it and is imposing a maintenance cost on the rest of the compiler). It sounds as if there's a fairly active community of m68k enthusiasts (I'm assuming that, in spite of the title of this thread, we are talking about the 68000, not the 6800), so I expect that the promotion to supported back end should go fairly smoothly. Note that, so far, I've been about the only person reviewing the CSKY patches and have very little spare bandwidth to do it at the moment. I'd encourage the m68k maintainers to take a look at it and help out as good community members. All of the back ends in LLVM would be better if more people reviewed code for back ends other than the one or two that they work on. David
John Paul Adrian Glaubitz via llvm-dev
2020-Nov-16 10:40 UTC
[llvm-dev] [RFC] Backend for Motorola 6800 series CPU (M68k)
Hello David! On 11/16/20 11:30 AM, David Chisnall via llvm-dev wrote:> Generally, the bar for being in-tree is fairly low, the bar to being removed > from the experimental-back-ends list is much higher. An experimental back end > is not built by default and is not in any of the binary releases. > > Experimental back ends provide a probation period for the maintainer community. > If bugs are fixed regularly, the back end makes good progress, the required > infrastructure is maintained, there's engagement from the maintainers in reviewing > other patches then it's easy to get the back end promoted to non-experimental. > If not, then it will languish in the experimental state for a while and then > eventually be removed (this will typically happen much faster if it requires > any infrastructure in the generic CodeGen logic to support it and is imposing > a maintenance cost on the rest of the compiler).Thanks for elaborating this and I think this is actually the way to go, for any such project. It's a fair deal between both the upstream project and the new maintainers. It allows us to get a fair chance, on the other hand, it still gives LLVM upstream the possibility to decide about the production readyness of the backend and consequently whether it's worth to keep or not.> It sounds as if there's a fairly active community of m68k enthusiasts (I'm assuming > that, in spite of the title of this thread, we are talking about the 68000, not the 6800), > so I expect that the promotion to supported back end should go fairly smoothly.Yes, despite its age, the architecture is still very popular. It has only little commercial relevance, unsurprisingly, but there is still new hardware being made and there are even new variants of the CPU being developed by the community, notably the 68080. For me personally, LLVM is an incredibly interesting and important project and I'm more than thrilled that we are getting a chance to upstream our backend into LLVM's repository. I'm currently trying to ramp up my personal effort to contribute to LLVM and make sure we're getting the backend supported on the targets we have in Debian. I have already set up a buildbot worker for linux-sparc64 that is being used and another one for linux-m68k is ready and waiting. I will later add one for MIPS (I've got a fairly fast Loongson board) and one for 32-bit PowerPC.> Note that, so far, I've been about the only person reviewing the CSKY patches and have > very little spare bandwidth to do it at the moment. I'd encourage the m68k maintainers > to take a look at it and help out as good community members. All of the back ends in LLVM > would be better if more people reviewed code for back ends other than the one or two that > they work on.I currently don't have the expertise to do so. But as I previously stated, I'm very interested to help keep LLVM in shape on the backends that are being used in Debian, especially the less popular ones as I know that ARM, PPC and X86 are very well maintained. Thanks, Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer - glaubitz at debian.org `. `' Freie Universitaet Berlin - glaubitz at physik.fu-berlin.de `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913