llvm dev - Nov 2020 - [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 F913
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.

* 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)

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, detailed
reviews, 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 the
writing 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>

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

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