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

Mon Mar 7 13:42:44 PST 2016

On Mon, Mar 7, 2016 at 1:31 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> On Mon, Mar 7, 2016 at 12:34 PM, David Blaikie via llvm-commits <
> llvm-commits at lists.llvm.org> 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.
>>
>
> Agreed. No harm in reverting, so let's get that done.
>

Not debating whether this is harmful or not,  'reverting' needs a good
reason -- not based on false statement the patch is not properly reviewed,
which is clearly is not the case.

thanks,

David

> That being said, I think that Chandler dropped the ball here. This patch
> has been on Phab for a month and a half. I would have expected Chandler to
> chime in with at least an ETA for when he would get a chance to look at it,
> given the importance of this patch.
>
> -- Sean Silva
>
>
>>
>> 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
>>>
>>>
>>
>> _______________________________________________
>> 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/f73aafcb/attachment.html>