[LLVMdev] RFC: LLVM incubation, or requirements for committing new backends

[Owen Anderson]

Tom,

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, 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.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120716/03870f14/attachment.html>