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

[Xinliang David Li via llvm-commits]

?? I have addressed the concerns and it seems there is a misunderstanding
there. The policy is pretty clear and there is nothing to be discussed.

David

On Mon, Mar 7, 2016 at 12:34 PM, David Blaikie <dblaikie at gmail.com> wrote:

> 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/add1687e/attachment.html>