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

[Chandler Carruth via llvm-commits]

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


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


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