<div dir="ltr"><div dir="ltr"><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 15 Nov 2020 at 21:59, Fāng-ruì Sòng via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I've looked at a few newer backends in LLVM and I think the policy is<br>
unclear about how an experimental target is approved and reviewed.<br></blockquote><div><br></div><div>Hi Fang-rui,</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Also worth noting that some of the initial review patches have just<br>
one or no reviewer on the reviewer list (i.e. probably not get enough<br>
scrutiny from community members).<br></blockquote><div><br></div><div>From what I saw, Craig, Roman and others were sending good, detailed reviews, so I refrained from comment. </div><div><br></div><div>Once everyone is happy and there aren't enough reviewers, let us know and we'll start pinging wider in the community.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
(In addition, our Phabricator setting makes a patch green if anyone<br>
accepts it - this makes it seem like the target is approved while the<br>
reviewer probably just intended that "the patch is in a good shape".<br>
Sometimes the author committed it in haste after accepted. Sometimes<br>
this may be understood as the author might have waited for too long<br>
before getting feedback)<br></blockquote><div><br></div><div>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. </div><div><br></div><div>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).</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Hope that helps.</div><div><br></div><div>cheers,</div><div>--renato</div><div><br></div><div>[1] <a href="http://llvm.org/docs/DeveloperPolicy.html#adding-a-new-target">http://llvm.org/docs/DeveloperPolicy.html#adding-a-new-target</a></div><div>[2] <a href="http://llvm.org/docs/SupportPolicy.html">http://llvm.org/docs/SupportPolicy.html</a> </div><div>[3] <a href="http://llvm.org/docs/CodeReview.html">http://llvm.org/docs/CodeReview.html</a></div><div><br></div><div>PS: If after all reviews you're still stuck and no one else is responding to your pings, feel free to contact me directly.</div></div></div>