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

[Philip Reames]

On 01/04/2015 05:32 PM, Chandler Carruth wrote:
>
> On Sun, Jan 4, 2015 at 4:56 PM, Philip Reames 
> <listmail at philipreames.com <mailto: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.
Chandler, I'm not going to argue with you on this one.   I disagree with 
your conclusions, but understand your reasoning.  I think the largest 
point we differ on is the effort/value trade off of separating two 
distinct changes.  Its not worth debating further.
>
>
> I thought I had built enough trust within the community to make this 
> kind of change without breaking thing significantly or causing problems,
This is a red herring and you know it.  :)  This isn't about me trusting 
you at all.  It's about having code which is easy to review and 
understand what is changing.
> 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 you may have misunderstood my point (which probably means I 
didn't make it well.)  The changes to split the analysis pass has been 
previously discussed and was uninteresting.  That's precisely why I 
would have liked you to separate the ownership change.  :)  I wanted to 
be able to review (pre or post is a separate debate) those changes in 
isolation without having to wade through the restructuring changes.

For the record, given it sounds like you had previously discussed the 
interesting part of this change with Hal, I will formally withdraw any 
objection.

p.s. Please do not hold future analysis restructuring for review.

Philip


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150105/39b5b8f0/attachment.html>