[PATCH] D21464: [PM] WIP: Introduce basic update capabilities to the new PM's CGSCC pass manager, including both plumbing and logic to handle function pass updates.

Sean Silva via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 7 14:29:56 PDT 2016

On Tue, Jun 28, 2016 at 5:55 PM, Chandler Carruth <chandlerc at gmail.com>

> chandlerc added a comment.
> In http://reviews.llvm.org/D21464#466098, @davidxl wrote:
> > Chandler, thanks for the tremendous effort for trying to make this work.
> At this moment, I only have very high level comments,  so please bear with
> me.
> >
> > Can we have a design document describing the underlying algorithm with
> formal proof (traversing with mutation)? Given the level of complexity
> involved and the fact there is no prior literature we can refer to, it is
> important we get it right.
> I've added an overview to the top of the file that I hope answers a lot of
> the high level questions.
> I don't have a formal proof of this, nor do I really know what you would
> want to prove here. What's the concern? I am working on building up a set
> of test cases that I think exercise the kinds of updates. I've not finished
> with that yet, but so far it seems to be hitting lots of the different
> aspects of this code.
> > Besides  it is also  more important to understand what problem the added
> complexity is trying to solve, why those problems are important to be
> solved (evidence of missing opportunities in real apps), and why they can
> not be solved in other ways?
> See my response to Sean and my comments in the updated patch (coming
> shortly) for some of the details here.
> I view this much more about getting the call graph management into a
> principled state with a path that supports the different goals. The new
> pass manager's requirement of *identity* for things to support analysis
> caching was a key element that drove the development of the LCG in the
> beginning. Without that kind of identity, it becomes extremely hard to
> reason about a caching approach to analyses. I can't claim its impossible
> to get something to work, but I tried and was unable to see a clear and
> clean path that made sense to me, so I worked on building up the
> infrastructure necessary to have identity.
> Once you have identity, you need to be very careful with updates because
> it makes it easy to transform a "benign" relaxation of the model (the old
> PM's approach of folding more nodes into the current SCC) into a
> correctness bug. As a consequence I've tried to take a very principled
> approach to the call graph update where we can verify and check that the
> graph structure has a particular shape and structure.

Chandler, can you please point to where in the code where "the old PM's
approach of folding more nodes into the current SCC" happens? I'm pretty
sure it does not happen, because doing that is precisely what one of the
changes in http://reviews.llvm.org/P6012 (quick hack) does, and I had to
make the `Nodes` member of CallGraphSCC public in order to do it (no
included in that patch for brevity).

-- Sean Silva

> Ultimately, I think this is the right way to design the call graph portion
> of the pass manager because I think other designs will inevitably run up
> against the limitations that their relaxed model imposes. In fact, I'd like
> to generalize and make more principled even the idea of the "reference
> graph" because I think it hasn't gone far enough yet, but that's for a
> future iteration.
> > The same questions have been raised in Sean's email thread, and there
> are some interesting discussions there, so may be you can also chime in
> there?
> That email thread happened when I didn't have time to read it as it went,
> and after skimming it I was unable to really understand what open questions
> there were.
> This *has* actually been discussed before, if you look at the discussions
> that led to the LazyCallGraph design. I don't know that we have a complete
> record of the discussion, but I've tried to capture the sentiment that
> folks were left with in terms of what direction the design should go in the
> comment I added to the top of the file.
> None of that is to say that we shouldn't revisit the design decisions if
> they are wrong of course! I just don't want to give the impression that
> there was *no* discussion about this. We should still try to figure out if
> there is a problem with this approach or a better approach that we should
> take.
> > As far as I can tell, for the new PM transition,  the consensus is to
> make the transition NFC initially, which will greatly lower the bar for the
> new PM to become the default.  If your plan is to make new PM transition
> depending on this update support in CGSCC pass to be ready,  we need to
> understand why.
> I don't actually agree with this. This has come up several times on
> several different threads with myself, Hal, Philip and numerous others.
> I think it would be *nice* if we could make the transition NFC, but I
> personally do not believe that is reasonable given the degree of difference
> between the designs. To me, it is very fundamental that when you have a
> caching analysis layer that understands the SCC organizational unit of IR,
> you need a different strategy for handling updates and iteration. Trying to
> force things to look exactly the same will, IMO, cause the design of the
> new pass manager to be worse.
> That said, we do have to have a way to migrate. My plan has been to simply
> make the new approach superior in terms of benchmark numbers, and at least
> not a significant regression in terms of compile time. Given the power of a
> more call graph aware refinement-driven iteration order, and the compile
> time savings of caching analysis, I think both of these will be reasonably
> attainable. However, if we try, and they prove very difficult, I think we
> will still be able to layer a constraint on top of the current design that
> more precisely mimics the old behavior in order to ease migration. I don't
> think it will come to this, and I also think that if it does, having what
> we think is the principled design (but which needs more work to address
> regressions) underneath will ensure that the compatibility mode sits
> cleanly on top and doesn't twist the core in an unfortunate direction.
> http://reviews.llvm.org/D21464
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160707/7a7df1fb/attachment.html>

More information about the llvm-commits mailing list