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

[Xinliang David Li via llvm-commits]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160307/6fe5c409/attachment.html>