<div dir="ltr">I just wanted to post a brief note her, this is not intended to be a full reply, but since you and a few others have asked I didn't want folks to not know why there was silence...<br><br><div class="gmail_quote"><div dir="ltr">On Thu, Jun 30, 2016 at 4:26 PM Xinliang David Li 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"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>I would still really love to talk about any concerns or problems you see with the design. Currently the one I'm thinking about the most is quadratic behaviors, but if there are other concerns I'd like to discuss them.</div></div></div></blockquote><div><br></div><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I may be repeating myself, but I would like to summarize my concerns and position here:</div><div><br></div><div>1. We have a goal to make the pass manager to use the new PM. There are many known/concrete performance improvements depending on that (there is possible short term solution that does not depend on new PM, but we've decided to make them depend on new PM switch anyway).  Note the PM switch is already a very large task with potential risks, coupling that task with another huge change is not the right plan going forward.  The right way is to make *minimal* changes enable new PM as soon as possible and make incremental improvement of other parts later.  The longer we delay, the longer the community suffers.</div><div><br></div><div>2. Changes with this scale needs to be justified with concrete benefit. If there are anything real missing, we should have known them already. </div><div>  </div><div>3. With changes in core infrastructure like this, subtle bugs are likely to be very hard to debug. When a bug is introduced, there will no way for user to workaround it (such as by disabling a pass). This is reason why core piece like this needs to kept simple</div></div></div></div></blockquote><div><br></div><div>FWIW, I think all of these comments apply much more to the overall effort around the new pass manager than this patch. We can re-visit those discussions if you want to do so, but I would like to separate out that discussion.</div><div><br></div><div>None of this is to say that we shouldn't try to keep the new pass manager simple, and streamline the process of getting it in place where reasonable.</div><div> </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"><div><br></div><div>4. The patch introduces a new SCC formation algorithm (double layer with support for CG mutation on the fly), however design document on how this works is missing.</div></div></div></div></blockquote><div><br></div><div>FWIW, this patch doesn't introduce any new algorithms like this. Those were in previous patches to the LazyCallGraph.</div><div><br></div><div>That said, I'm very happy to try to provide a document around this. Much of this was discussed extensively a long time ago when I originally worked on LCG. Now....</div><div> </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"><div>  It needs to document </div></div></div></div></blockquote><div><br></div><div>Yes, these discussions did not all materialize in a document. Some did, but not enough. I'm working on these but it will take some time. Also, it is somewhat orthogonal to *this* patch, except (as you point out) that it will make review of this patch easier.</div><div><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"><div><span style="line-height:1.5">5.  Another added complexity is that the SCC passes are now required to do the updates -- it seems pretty obscure to the pass writers.</span></div></div></div></div></blockquote><div><br></div><div>This is already true though. PruneEH, the Inliner, and ArgumentPromotion all update the call graph as they mutate the IR.</div><div><br></div><div>That said, it is a concern. With the old pass manager we skipped making certain kinds of updates (SCC refinement is an example) and not being able to skip those updates will add some complexity. However, I think we can effectively provide utilities and common code to share that logic to the extent possible. Some of this is already in LCG in fact.</div><div><br></div><div>I can imagine some of the code in this patch will get refactored in order to be reused by other passes, but I didn't want to do that until I knew how the other passes would actually use the code.</div><div> </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"><div><span style="line-height:1.5">6. What is the plan to address the quadratic behavior?</span></div></div></div></div></blockquote><div><br></div><div>I've added comments about this to the header. I don't have a single plan, because there are several that seem very viable. One that I'd like to see if it is sufficiently easy to do is to move to using the existing globals graph as a form of indirection in the reference graph. That would cause the graph to more closely match the IR and has the potential to open up some really nice opportunities around GlobalOpt. But if that isn't easy to do, there are several alternatives such as artificially building a layer of indirection to cope with the potential for quadratic size.</div><div> </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"><div><br></div><div><br></div><div>7. Is this tested on large programs (e.g, in LTO mode)?  How large is the RefSCC graph? How much memory overhead is expected? What is the compile time impact?</div></div></div></div></blockquote><div><br></div><div>With any indirection around the reference graph to prevent duplicate edges that don't exist in the IR, this is going to use much less memory than the current call graph which is quite wasteful of memory. But I've not seen any instances where the memory usage of the current call graph was a critical part of our memory concerns, so I've mostly been focusing on removing the one known source of quadratic growth (which I think is not scary here, but if that's what is giving people pause, I'm happy to prioritize it).</div><div> </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"><div><br></div><div>8. This change makes new PM transition not NFC.  It can potentially affect a lot of other people, e.g., introduced performance regression, correctness issue etc.</div></div></div></div></blockquote><div><br></div><div>This is definitely true, but has actually been discussed before. I want to point out that, the new PM is *already* not NFC. There are places where the old pass manager has hacks that work around a lack of available analyses (FunctionAttrs using AA for example).</div><div><br></div><div>I'm not sure making the pass manager NFC is a reasonable or interesting goal. I'm much more interested in just making it *better*.</div><div> </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"><div><br></div><div>So in short, my proposal is that we do minimal work in CGSCC pass manager to enable/unblock porting of inliner to the the new PM, and we can revisit/re-examine this patch in depth later when new PM is in place.  Does it make sense?</div></div></div></div></blockquote><div><br></div><div>I think the core issue is whether there is a simpler approach that actually works and doesn't just introduce fragility into the system. I wasn't able to find a good approach. Maybe Sean has, and if so, great. That, IMO, doesn't mean we should stop working on the approach in this patch.</div><div><br></div><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">My plan going forward is to work on the design documents that are very reasonable to explain how pieces of this work, and on looking at Sean's approach to understand it.</span><br></div></div></div>