<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, Jul 7, 2016 at 2:30 PM Sean Silva via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 28, 2016 at 5:55 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">chandlerc added a comment.<br>
<span><br>
In <a href="http://reviews.llvm.org/D21464#466098" rel="noreferrer" target="_blank">http://reviews.llvm.org/D21464#466098</a>, @davidxl wrote:<br>
<br>
> 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.<br>
><br>
> 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.<br>
<br>
<br>
</span>I've added an overview to the top of the file that I hope answers a lot of the high level questions.<br>
<br>
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.<br>
<span><br>
> 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?<br>
<br>
<br>
</span>See my response to Sean and my comments in the updated patch (coming shortly) for some of the details here.<br>
<br>
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.<br>
<br>
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.<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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 <a href="http://reviews.llvm.org/P6012" style="font-size:12.8px" target="_blank">http://reviews.llvm.org/P6012</a> (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).</div></div></div></div></blockquote><div><br></div><div>Sorry, I was remembering the ReplaceNode functionality that ArgumentPromotion uses to mutate the SCC. You're correct that we don't add nodes to the SCC.</div><div><br></div><div>In summary, we neither remove nodes from an SCC when cycles are broken nor do we add nodes to the SCC when we introduce a cycle. The latter is conservatively correct (I think) because the SCC was modeled as having an indirect call which is more conservative than if we merged a concrete node into the SCC and treated it as a direct call.</div></div></div>