[llvm-dev] [RFC] Backend for Motorola 6800 series CPU (M68k)

[Renato Golin via llvm-dev]

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>