<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jul 7, 2016 at 11:18 AM, Mehdi Amini via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</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"><div style="word-wrap:break-word"><br><div><span class=""><blockquote type="cite"><div>On Jun 30, 2016, at 4:26 PM, Xinliang David Li via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:</div><br><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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"><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>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></div></div></div></blockquote><div><br></div></span><div>I agree (and I suspect everyone does), but this ship sailed more than 2 years ago :)</div><div>This should have been an initial process goal, but the current situation is that most of it is implemented as far as I can tell, and I wouldn’t vote for Chandler to stop his work while being close to land it.</div></div></div></blockquote><div><br></div><div>This patch may be "close to land" (although that is arguable), but we haven't seen what kinds of changes this implies for e.g. the inliner. From my experience preparing D21921, I'm skeptical about the ability to reasonably maintain the inliner with Chandler's new stuff coexisting with the old PM. I would need to see a proof of concept that is sane before LGTM'ing Chandler's changes here (ignoring all the other points that David/Pete have made about doing this incrementally, testing all the various cases, getting a design document, etc.).</div><div><br></div><div>On the other hand, the new PM's design makes it relatively easy for Chandler's stuff (once we have proper design docs, testing, PoC for the inliner, etc.) to live alongside the approach in D21921 if some people feel strongly that work for that needs to be done immediately (something that I disagree with, but for the sake of reaching consensus would not oppose).</div><div> </div><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"><div style="word-wrap:break-word"><div><div>(At the same time, I like Sean plan to get in production the new PM with the current visitation order)</div><span class=""><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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></div></blockquote><blockquote type="cite"><div><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.  It needs to document </div><div>    * What exactly is expected (in terms of vistation order) when a) an edge is added; b) an edge is deleted; c) when a new node is introduced and connected. </div><div>    * How ref-edges are formed, how indirect callsites are handled etc.</div><div>    * If it is a modification of the classic SCC formation algorithm, describe the change and prove that the algorithm works as expected.</div><div><br></div><div>Having a document like this also helps reviewers of the code (for reasoning of the correctness).  Without this, I suspect very few people can help with maintaining the code in the future.</div></div></div></div></div></blockquote><div><br></div></span><div>I Agree.</div><span class=""><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>5.  Another added complexity is that the SCC passes are now required to do the updates -- it seems pretty obscure to the pass writers.</div></div></div></div></div></blockquote><div><br></div></span><div>We don’t have much SCC passes though, and I guess we can come up with some nice way to express it.</div></div></div></blockquote><div><br></div><div>Existing CGSCC passes already do updates. The hard part here is sharing code that does updates when the semantics of the updates are different in the old PM and the new PM. Even just look at PostOrderFunctionAttrs and how much code had to be duplicated, and that pass doesn't even mutate the call graph. It is going to be hard to paper over this and I would have to see a proof of concept to convince me otherwise.</div><div><br></div><div>Or to summarize: I think the obvious incremental approach here is to start by using the old PM visitation logic, delete the old PM from the middle-end, then make changes to the visitation semantics without being constrained by coexisting with the old PM.</div><div>Does anyone disagree with that?</div><div><br></div><div>-- Sean Silva</div><div> </div><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"><div style="word-wrap:break-word"><div><span class=""><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>6. What is the plan to address the quadratic behavior?</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><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><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></div></blockquote><div><br></div></span><div>Does it line up with Sean’s work?</div><div><br></div><div>— </div><div>Mehdi</div><div><br></div><br><blockquote type="cite"><div><span class=""><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>thanks,</div><div><br></div><div>David</div><div> </div><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"><div dir="ltr"><div class="gmail_quote"><span><div><span style="line-height:1.5"> </span><br></div><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><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"><div dir="ltr"><div class="gmail_quote"><span><div> </div><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> Regardless, if/when the evidence appears for those cases being important to optimize, we can add them; porting to the new PM is orthogonal.</div><div><br></div><div>[*] Hopefully I will start an RFC and send out a proof of concept patch tomorrow; I just need to test out a proof of concept port of the inliner on something bigger to make sure everything works.<br></div></div></div></div></blockquote><div><br></div></span><div>As I have said in this thread, several other threads when LazyCallGraph was first designed, and in several other discussions, I continue to think this is the wrong approach.</div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Can you provide links to those discussions? I can't find anything in my email.</div></div></div></div></blockquote><div><br></div></span><div>I have tried very hard to find them, and haven't succeeded. Really sorry about this. I know some of them took place outside of email sadly, but my memory is that some did take place in the discussion threads around the pass manager and the need for having a cache key for SCCs.</div><div><br></div><div>That said, my only real goal in mentioning this is that I don't want you or others to think that this is something that was never discussed. I even clearly remember Hal having reservations about this. ;] I know I myself would have preferred to maintain precise similarity to the current pass manager, but I don't see a way that results in a robust interaction with cache based management of analyses.</div><div><br></div><div>Anyways, I'm happy to try to explain this freshly if useful, but I'm not sure what the remaining points of contention or confusion are.</div></div></div>
</blockquote></div><br></div></div></span><span class="">
_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br></span></div></blockquote></div><br></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div>