[llvm] r225131 - [PM] Split the AssumptionTracker immutable pass into two separate APIs:

[Hal Finkel]

----- Original Message -----
> From: "Chandler Carruth" <chandlerc at gmail.com>
> To: "Philip Reames" <listmail at philipreames.com>
> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Sunday, January 4, 2015 7:32:12 PM
> Subject: Re: [llvm] r225131 - [PM] Split the AssumptionTracker immutable pass	into two separate APIs:
> 
> On Sun, Jan 4, 2015 at 4:56 PM, Philip Reames <
> listmail at philipreames.com > wrote:
> 
> 
> Chandler, this is large enough it really should have gone for review.
> At minimum, a chance for everyone involved to process the ownership
> changes. I'm not asking you to revert, just making the point.
> I really disagree.
> 
> 
> First off, this change is not actually large in substance. Almost all
> of the diff is entirely mechanical.
> 
> 
> For the changes to the core analysis, I actually did discuss them
> very briefly with Hal on IRC, but they didn't seem controversial
> enough to warrant deep review. There were two substantive parts to
> it:
> 
> 
> - The change to have a separate analysis API from the pass was
> already discussed numerous times in the context of the new pass
> manager. I don't think its reasonable to re-hash that discussion
> with each analysis.
> 
> 
> - The change to use a vector instead of a set of the assume
> intrinsics, and weak value handles rather than self-removing
> callback handles in that collection. I wouldn't have made this
> change at the same time at all except that as I started to do the
> mechanical bit, it became obvious that this would be a significant
> simplification. I would have had to do much more work to separate
> them than I had to do land them at the same time.
> 
> 
> I do regret the fact that a design change only became apparent half
> way through a large, mechanical refactoring to separate out the
> cache from the pass. But it really didn't seem worth going through
> the very significant amount of work to separate them in this
> instance, and the value of separating them just didn't seem that
> high -- this is not a complex or subtle piece of code, and the
> changes just didn't seem that interesting.

I agree, this change is largely mechanical. The relevant design changes were discussed with me before-hand, and I'm comfortable with reviewing the work post-commit.

> 
> 
> I thought I had built enough trust within the community to make this
> kind of change without breaking thing significantly or causing
> problems, and the nature of the change seems obvious to me. You
> clearly disagree, but I'd like some confirmation from others on this
> front. I'm going to be make many similar changes to this one as part
> of porting every pass to work with both pass managers. If they all
> need pre-commit review, then frankly, it's not going to happen. I'd
> have better luck creating a branch and doing it there.

I think this is a bit strong, it depends on how quickly reviewers approve things ;) -- but I agree it would take longer this way. That having been said, I'd prefer that we stick to post-commit review for this work. The overall design being implemented has been discussed a lot, both on this list, in multiple dev meeting talks, etc. and it lies on the critical path for more essential optimizer improvements than I can shake a stick at (figuratively speaking, likely). There are certainly enough interested parties that the code will be looked at in detail: I'm not concerned about that. If something is really problematic or controversial, we'll revert it.

 -Hal

> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory