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

Tue Jul 17 05:45:51 PDT 2012

On 07/16/2012 03:47 PM, Tom Stellard wrote:
> 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.
The NVPTX case was unique in that it was replacing an existing back-end, 
so there was an established track record with the back-end and its 
maintainers.

I really think the main problem with getting this back-end upstreamed is 
its size, and its unique usage scenario (e.g. no assembly, just binaries 
that can only be consumed by Mesa/Gallium). It may help to provide some 
documentation on the usage of this back-end, perhaps with a simple 
example showing an LLVM IR compute kernel, and some human-readable 
output of the back-end.

When I originally looked at it a couple of months ago, I was put off by 
test cases which seemed to only test a few very specific things.

I have to admit that I was playing with it awhile ago and was confused 
why llc was producing no output, until I realized that it *was* 
producing a binary form that my terminal was just swallowing. :)

>
>> 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
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits