[llvm-commits] RFC: LLVM incubation, or requirements for committing new backends

[Duncan Sands]

Hi Owen,

> I think it might be productive to fork this thread to discuss making the
> requirements for upstreaming a new LLVM target more explicit and open.  I'd also
> like to gauge interest in an idea I've discussed privately with a few community
> members, namely the concept of having a semi-official "incubation" system
> whereby proposed backends could get a trial run before becoming part of LLVM
> mainline.
>
> The proposed system would be something like this: a proposed contribution would
> receive a branch on llvm.org <http://llvm.org/>, and have six months (or some
> other predetermined length of time) to demonstrate that it and its developers
> are ready for mainline integration.  At the end of their term, incubated
> projects would be evaluated on the following criteria, and either integrated to
> mainline, judged to be more appropriate as an external project, or given an
> extension and "needs improvement" feedback on specific criteria.

so this would be something like the linux kernel's "staging" tree?  Well, why
not.  However another important thing which I think should be extended is the
notion of "code owners" (corresponding more or less to the linux kernel's
subsystem maintainers).  Who is the code owner for backends in general?  Maybe
Evan or Anton?  A new backend shouldn't go in unless OK'd by that code owner.
But who knows who the right person is?  I think we need to designate code owners
for all major subsystems, and list them in some easy to find place.  One problem
people have when they send in patches but don't get any review is that they
don't know who to turn to.  They should turn to the code owner, and part of the
code owner's responsibility should be to take action in such cases (which may
just mean delegating the review work to someone else).  Currently they often
turn to Chris, but Chris doesn't scale.

Ciao, Duncan.

PS: I agree with your other points below.

>
> * Active maintainership - Backends bit rot quickly, and unmaintained backends
> are large maintenance burden on everyone else.  We /need/ a core of developers
> who are going to actively maintain any candidate backend /on mainline/.  That
> last point is critical: a code drop every six months is /not/ an acceptable
> level of maintenance for a mainline target.
>
> * Contributions to core - Mainlining a new backend adds the expectation that
> mainline LLVM developers will invest the time and energy to keep your backend
> building and working (see test plan, below).  However, that expectation of extra
> work doesn't come for nothing: we expect you to contribute back fixes and
> improvements that you find, and to work with other community members to
> coordinate projects as appropriate.  When looking at a new backend, I should
> expect to see few-to-no diffs outside of lib/Target/YourBackend, and a few other
> places (Triple.cpp, for example).  All other changes should already be upstreamed.
>
> * Test plan - If you're going to expect us to maintain and fix your code, then
> you need to have a good answer to how to test it.  This includes, but is not
> limited to, a good set of regression tests that are comprehensible to normal
> developers (so we can fix them when they fail due to mainline change), and
> continuous testing in the form of buildbots or other infrastructure (so we can
> know when a patch breaks your backend).
>
> * Up to date with mainline - All mainline backends must work with top-of-tree
> LLVM, all of the time.  A candidate for inclusion /must/ be developed at, or
> close to, mainline.  In practice, that probably means updating at least once a
> week, possibly more.
>
> * LLVM coding standards - While small deviations can be fixed after mainlining,
> gross violations of the LLVM code standards and conventions must be fixed prior
> to integration.
>
> ---
>
> So, what would the community think of implementing such a system?
>
> --Owen
>
> On Jul 16, 2012, at 10:57 AM, Chandler Carruth <chandlerc at google.com
> <mailto:chandlerc at google.com>> wrote:
>
>> Tom, I have to ask that you revert this.
>>
>> As we discussed a long time ago, and as I explained in great detail to the
>> Intel folks working on x32 support[1], we simply cannot accept really
>> significant additions to the codebase without active, trusted maintainers who
>> have an established track record contributing and maintaining LLVM's code. For
>> the reasons why this is so important, I would read the x32 email.
>>
>> [1]:
>> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120709/146241.html
>>
>> I can't emphasize this enough: established maintainers with a track record. I
>> really do know how high a barrier to entry this is, it's excruciating. We've
>> gone through this multiple times. However, without this, the project and the
>> codebase simply cannot scale.
>>
>> Next, there is a further problem here: this patch went in without review. That
>> is unacceptable for a contribution of this magnitude. I realize that in the
>> past there may be examples where this rule has not been applied well, but that
>> does not invalidate it or exempt you from it. I'm particularly frustrated
>> because you *knew* this was a requirement and committed anyways.
>>
>> Finally, there are really deep problems with your contribution as posed. I'll
>> touch on a few of them here, but by no means should you consider this an
>> exhaustive list:
>> 1) You must have an AsmPrinter. You must properly use and support the MC
>> layer. This layer is no longer experimental or poorly supported, every new
>> backend should be expected to implement proper MC support.
>> 2) You need to write tests that follow the prevailing LLVM style. This
>> includes a textual input and output, with FileCheck to manage detailed and
>> robust assertions.
>> 3) You need to consistently leverage the modern elements of the target
>> independent code generator. I haven't done a deep study of this backend, but a
>> cursory look indicates that you're not properly integrating with some of the
>> latest additions no the target independent pipeline. Adding a new backend that
>> doesn't support them greatly magnifies the cost of making changes to this
>> common infrastructure.
>> 4) You must bring the code up to the coding standards and style of LLVM. I
>> don't know why people find this so challenging. Look at recently added LLVM
>> code in the backend, look at the patterns and style it follows, and *exactly*
>> replicate it. You're not even close, considering the patch contained 'or'
>> instead of '||'.
>> 5) High quality documentation about the backend, the target platform, and your
>> plans here.
>> 6) An active build bot to help developers without access to your platform
>> debug issues.
>>
>> I realize that not every backend meets this bar. That doesn't imply that your
>> backend doesn't need to meet this bar. We have to raise the quality bar if
>> we're going to keep LLVM moving forward at a rapid pace. Currently, we aren't
>> doing that, and it is costing the project a great deal.
>>
>> ====
>>
>> On a separate note, I truly understand that getting a review for a patch of
>> this magnitude is hard. You are not alone in wanting a review and not getting
>> it. However, submitting without review does not solve anything, it merely
>> takes more time from reviewers to deal with the problems in a rush, and makes
>> every single reviewer less inclined to actually review your patch thoroughly.
>>
>> You need to specifically motivate people to review your patch. There is no
>> other way you can get it into the tree. There are many ways to do this:
>> 1) Make it's code so excellent in quality and familiar in style to the
>> potential reviewers that they actually enjoy it.
>> 2) Work tirelessly on fixing and improving the core LLVM infrastructure so
>> that potential reviewers are grateful and motivated to keep you active in the
>> project.
>> 3) Talk to developers in the community, showcase amazing things that this
>> backend will do or let you do when it is in the tree.
>> 4) More of 1, 2, and 3.
>>
>> The only magic I know of is to submit more patches. To submit so many patches
>> that other developers simply cannot ignore your presence and will have to
>> review your code. The currency of patches does actually work in this project,
>> but you haven't yet invested enough.
>>
>> ====
>>
>> I truly hope you don't take this to mean that I (or others in the LLVM
>> project) am uninterested in this backend eventually being in the tree. We are
>> interested, but it's not ready yet. We need you (or others) to be much more
>> active in maintaining things in LLVM. We need the quality of the code and
>> implementation and testing to go up. We need it to go through proper code
>> review. That is the context in which we are interested.
>>
>> -Chandler
>>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>