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

[Chandler Carruth]

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