<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Jul 1, 2016 at 2:36 PM Pete Cooper 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="auto"><div>FWIW, I agree with all of David's points here.</div></div></blockquote><div><br></div><div>I've replied to that email, see my responses there.</div><div> <span style="line-height:1.5"> </span></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div><span style="line-height:1.5">Also, we don't tend to like 2600 line patches. Chandler you have often asked people to find a way to land things incrementally so I think that is appropriate here.</span></div></div></blockquote><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">I have broken this up to the greatest extent I was able to. I'm happy to have suggestions for further splits (will reply below).</span><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div><span style="line-height:1.5"> You've also asked in the past for detailed design and analysis which I know you are providing more of as questions arise, but something of this magnitude needs plenty of discussion. If anything, you should continue the discussion on llvm-dev until everyone is happy with the design, *then* come back to this patch.</span></div></div></blockquote><div><br></div><div>So, there were several discussions back when the LazyCallGraph was written. That was some time ago, and so I understand folks don't recall them (I don't recall all of them!). I'm working to capture them and put them into a document. =] No need to +1 that a million times.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div><br></div><div><br></div><div>If you do prefer to continue this route then fair enough. To make it an incremental series of patches, I propose the following:</div><div>- implement the most basic Ref SCC generator</div><div>- write unit tests to show it works</div><div>- land that patch</div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div>- Implement the refCC to SCC visitor, unit test and land that</div><div>- add a unit test which fakes various kinds of inliner updates to the graph.</div></div></blockquote><div><br></div><div>You might take a look at the unit tests for the LazyCallGraph. That code went in incrementally, and with unit tests exactly as you describe to exercise the abstract graph changes.</div><div><br></div><div>This patch is different. It is handling exactly *one* update scenario of running function passes.</div><div><br></div><div>I understand that it is more code than people would naively expect. I share that feeling. Even the old call graph's analog to this logic is 200 lines of code and it handles fewer cases. =/</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div> Basically try to cover all of the updates you think will be required by the inliner. Each one can be reviewed to make sure it's a valid kind of update that folks think makes sense</div><div>- same again with devirt code, updates, and tests</div><div>- finally you should have enough in place and everyone should be happy that the unit tests cover everything that we can port the passes.</div><div><br></div><div>I think something like the above gives us a much more incremental route to the goal. People can review each piece individually instead of one large patch.</div></div></blockquote><div><br></div><div>I think this is actually trying to follow the exact approach you've outlined. I understand that the result may be somewhat ... frustrating. It was for me. But I'm not sure how to effectively split up this logic.</div><div><br></div><div>For example, separating the devirt changes from the DCE changes don't make a lot of sense -- they're both intrinsically just "function local changes occurred" and are likely to be handled by the same piece of code. I've actually separated out the inliner changes from this one. I'm hoping to do that in a follow-up patch.</div></div></div>