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

Mon Mar 7 16:03:51 PST 2016

Thanks Hal for a good summary on the issues involved. I'll revert the patch
and hope to get the patch reviewed soon by you,  Chandler and anyone else
interested in this.

- Easwaran

On Mon, Mar 7, 2016 at 3:53 PM, Hal Finkel via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
> ------------------------------
>
> *From: *"Xinliang David Li via llvm-commits" <llvm-commits at lists.llvm.org>
> *To: *"Sean Silva" <chisophugis at gmail.com>
> *Cc: *"Junbum Lim" <junbuml at codeaurora.org>, "Prashanth N R" <
> prashanth.nr at gmail.com>,
> reviews+D16381+public+5d338da8ee48790f at reviews.llvm.org, "llvm-commits" <
> llvm-commits at lists.llvm.org>, twoh at fb.com
> *Sent: *Monday, March 7, 2016 3:42:44 PM
> *Subject: *Re: [PATCH] D16381: Infrastructure to allow use of PGO in
> inliner
> 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.
>
>
> Unfortunately, we have several intertwined issues here:
>
>  1. After David and Easwaran spent a significant amount of time reviewing
> and revising this patch (in addition to others who also commented),
> including testing it, Chandler wrote, "Please revert this and let's
> actually get it reviewed before it goes into the project." I can see how
> this can come across as insulting, as David certainly felt as though he did
> "actually" review the patch before it went into the project.
>
>  2. Despite being cc'd on the patch review thread for several months,
> Chandler did not comment on the patch one way or the other.
>
>  3. Chandler is currently our foremost expert in this part of the
> optimizer, and furthermore, his active pass-manager work is strongly
> connected with the issues with which this patch deals. In addition, he is
> the relevant code owner.
>
>  4. David ok'd the patch, and I agree with Chandler that this is a major
> change, without direct input from Chandler or any other contributor who has
> significantly worked in this area.
>
>  5. We generally honor revert requests for any reason, and the code owner
> being uncomfortable with the direction taken in the patch is a valid
> reason. Post-commit review is fine for fixups, but for underlying design
> questions, reverting is appropriate. As such, the patch should be reverted
> pending the associated design discussion and/or review (which we now all
> expect should take place in short order).
>
> Thanks again,
> Hal
>
>
> 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
>>>
>>>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
> _______________________________________________
> 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/f357d4b4/attachment.html>