[llvm-commits] [llvm] r160270 [1/2] - in /llvm/trunk/lib/Target/AMDGPU: ./ MCTargetDesc/ TargetInfo/

[Tom Stellard]

Hi Chandler,

On Mon, Jul 16, 2012 at 10:57:34AM -0700, Chandler Carruth wrote:
> Tom, I have to ask that you revert this.
>
Done.
 
> 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.
> 

I will be sure to review this email.

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

You're right.  Committing was a mistake, and I'm sorry for that.

Hopefully, you can understand where I was coming from.  Over the last
three and half months, I have submitted five versions of this backend for
review, and I received only one response with comments over that period.
Despite the absence of comments, myself and others, were working hard to
improve the quality of the backend.

After waiting so long for reviews, I started to question the approach I
was taking towards getting the backend upstream, and I thought maybe I
was doing it the wrong way.  So, I did some research on other backends
to see what the review process was like.  In particular, the review
process for the NVPTX seemed to proceed in manner that is more familiar
to me coming from other Open Source projects, which is, request
comments, address comments, and then commit when there are no more
comments.  So, I decided to try that approach with this backend.

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

Thank you for taking a look, I will try to address these concerns.

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

This is certainly a very high bar, and I agree that deviations from the
standards in the past don't justify deviations in the future, but I hope
you can understand how it can be frustrating to see other backends which
are accepted in significantly less time than this one.

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

Myself and others have invested a lot in preparing this backend for
inclusion.  You can look at the Mesa commit log to see the work that has
gone into it
http://cgit.freedesktop.org/mesa/mesa/log/?qt=grep&q=radeon%2Fllvm.

My time is limited, but I have been working to become a contributor to the
project.  I try to answer mailing list questions that I know the answer
to, and I've submitted two patches to shared areas of LLVM.  However,
this backend is a community project, so it's not my contributions that
are important, but the contribution of the entire community surrounding
the backend.  There really aren't enough community resources to 'work
tirelessly' on improving the LLVM core and also make improvements to
the backend, so I'm a little concerned it will be difficult to meet such
high expectations.

Also, please understand that since this backend is an Open Source
project, unlike most groups, we can't just role our backend into a
binary package and distribute it to our users.  We either need to have
it in the main tree or maintain our own public open source fork of LLVM.
We are currently doing the later, and as the maintainer of the backend,
I really don't want to be in the situation where distros start wanting
to package our version of LLVM to get access to AMD Open Source compute
drivers.  This is why I'm motivated to get this backend upstream.

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

I'm glad that the LLVM project is interested in this backend, but
if the LLVM developers are serious about getting it added, then it has
to be a team effort, with LLVM developers contributing reviews and
backend developers contributing code.

Again thanks for taking the time to look at the code.  I apologize for
the circumstance, and I hope we can all work together to get this
backend upstream.

-Tom Stellard