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

Mon Mar 7 15:29:26 PST 2016

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

>
>
> On Mon, Mar 7, 2016 at 1:42 PM, Xinliang David Li <davidxl at google.com>
> wrote:
>
>>
>>
>> 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.
>>
>
> This is not really about whether the patch was "reviewed" and quibbling
> over that definition or what the "letter of the law" says.
>
> I think that most experienced community members would agree that it was
> premature to commit this without
>

Normally I agree with you 100% on this assuming parties involved are
reasonable.

This is a special case here. The community has gone a really long way to
achieve this.  It is not like it has not being discussed and came a
surprise to anyone. On the contrary it has been discussed extensively in
various contexts in the past.

1) from my record, the first time it was brought up is ~ 1.5years ago --
where Ivan from Qualcomm has brought up the issue and solutions were
discussed:

December 26, 2014
http://lists.llvm.org/pipermail/llvm-dev/2014-December/080071.html

2) we had an official proposal to address PGO related issues in LLVM
(including the plan and method to integrate profile with inliner similar to
this patch) on Feb 24, 2015, a little more than a year ago:

http://lists.llvm.org/pipermail/llvm-dev/2015-March/082932.html

3) On June 17, 2015, Philip brought the same topic in llvm-dev list and we
had extensive discussions there:
http://lists.llvm.org/pipermail/llvm-dev/2015-June/086894.html

4) On Dec 7, 2015, A patch was submitted to workaround the issue of lacking
profiling data in inliner -- see response from Hal about what the right
approach is

5) Then it is this patch -- it has been thoroughly discussed for over a
month that implements the approach discussed before.

At this point, I think it is way more productive if people who are
interested can participate in post-commit reviews (which are greatly
appreciated)! We should focus more on the technical side of the patch.
Risking delaying this patch for another long/unspecified period of time
brings the community no benefit.

Hope this sounds reasonable.

thanks,

David

> some sort of explicit acknowledgement from Chandler that he had at least
> taken a look at the patch (from the public thread, it seems like it may
> have gotten lost in his email or something).
> Chandler is not without blame for the delay (unless his email swallowed
> this thread or something), but that should have been met with more vigorous
> pinging, possibly escalating to a PM / internal communication.
>
> However, It is understandable that a reading of the developer policy might
> lead to a different understanding of the expectations, so the mistake is
> completely understandable. Let's just revert and start a fresh review; if
> everything is ok with the patch the review should be quite short.
>
> If you want, we can try to enunciate a bit more clearly what the general
> community expectation is here so that we can add it to the developer policy.
>
> -- Sean Silva
>
>
>>
>> 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/742c6b81/attachment.html>