[PATCH] D16381: Infrastructure to allow use of PGO in inliner

[David Blaikie via llvm-commits]

This isn't really the place to debate LLVM policy - the code owner asked
you to revert a patch due to concerns over the review. Please revert it &
take up the discussion about general policy on llvm-dev, or the discussion
about whether the revert is merited after reverting. There's really no harm
in reverting a patch.

On Mon, Mar 7, 2016 at 10:17 AM, Xinliang David Li via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
>
> On Mon, Mar 7, 2016 at 9:58 AM, Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
>> On Mon, Mar 7, 2016 at 6:47 PM Xinliang David Li via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> On Mon, Mar 7, 2016 at 1:40 AM, Chandler Carruth <chandlerc at gmail.com>
>>> wrote:
>>>
>>>> chandlerc added a comment.
>>>>
>>>> In http://reviews.llvm.org/D16381#367348, @davidxl wrote:
>>>>
>>>> > LGTM.
>>>> >
>>>> > This looks really great, so let's move on with this long waited
>>>> missing feature.
>>>>
>>>>
>>>> David, this is not an area of LLVM you have done substantial work on,
>>>> and this is a very significant feature.
>>>>
>>>
>>> The patch has been reviewed and tested thoroughly. The approach used by
>>> the patch has been discussed many times in the past since last October in
>>> various contexts in which you were involved.
>>>
>>>
>>>>
>>>> I'm sorry that I have not had time to review this yet, but the correct
>>>> response is not for you to make a patch as LGTM.
>>>
>>>
>>>
>>> Again we have done extensive review on this patch. According to your
>>> standard, it looks like only you can approve patches in the inliner?
>>>
>>
>> Nope, but the only people on this thread are you and Easwaran.
>>
>>
> There are 4 reviewers listed (including you) and 7 subscribers including
> llvm-commits.  Click on 'Show older changes' to see the review history.  So
> what exactly do you mean the only people on the thread are me and Easwaran?
>
>
>
>> There are other folks who I think could reasonable review changes to the
>> inliner, but you've not gotten a review from any of them... Hal, Philip,
>> Sanjoy, or several others would all potentially be reasonable people.
>>
>
>
> You and Hal are listed as reviewers, and the patch were pinged several
> times.
>
>
>>
>>
>>>   IMO this is not healthy to the project and more people need to get
>>> involved. As it stands today, it seems to be very hard to everybody in the
>>> community to make any changes in the inliner due to this.
>>>
>>
>> Note that it is very hard to make changes to the inliner in general. It
>> isn't clear that a small number of reviewers for inliner changes is a
>> substantial issue for the community in general.
>>
>>
>
> Why do you think it is hard to make changes to inliner?   As far as I
> know, this feature is a *long* waited feature by the community. People who
> care have been following.  This patch is a great progress forward for the
> project not the other way around. If you see substantial issue, please be
> specific.
>
>
>>
>>>
>>>
>>>> Please revert this and let's actually get it reviewed before it goes
>>>> into the project.
>>>>
>>>>
>>> Actually get reviewed?  What do you mean? Have you really followed the
>>> thread?   If you want to be constructive, please do your part of the
>>> review, but not request a revert like this.
>>>
>>
>> The patch needs to be reviewed by someone who has significant experience
>> working on LLVM's inliner, as it is a significant change.
>>
>
>
> The patch is very nicely structured and it is not a significant change. We
> have been very careful in designing and testing.   Besides both Easwaran
> and I have extensive experience in inliner design in general.
>
>
>> The patch shouldn't stay in the tree just because the review is slow, as
>> that doesn't actually solve anything. Everyone else is working with
>> reviewers to get their patches reviewed, and I don't think it is reasonable
>> for you to just push forward. =/
>>
>
> Let me repeat, the patch has been carefully reviewed. I am not saying that
> all problems have been covered, but it is in reasonably good shape before
> it is approved.  We are not rushing it in -- the related discussion has
> been going on for half a year!
>
> Only more tests from the community can help reveal more problems -- if
> there are problems, we can always fix them.  Have you seen actual problem
> to the community by this patch?
>
> David
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160307/eccc91ca/attachment.html>