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

[Hal Finkel via llvm-commits]

----- Original Message -----

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