<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jun 28, 2016 at 10:01 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 inline comments.<br>
<br>
================<br>
Comment at: lib/Analysis/CGSCCPassManager.cpp:327<br>
@@ -24,1 +326,2 @@<br>
+  return *C;<br>
 }<br>
----------------<br>
<span class="">>> As i somewhat alluded to above, what you describe should be handled by a ref edge turning into a call edge, and the update mechanism should be able to handle that well.<br>
><br>
> Please add test cases exhibiting this.<br>
<br>
</span>Er, the next sentence in what I wrote was:<br>
<span class=""><br>
>> I've added a test case that exercises this with GVN and function-attrs.<br>
<br>
</span>I think the updated patch has this test case in it. It is cgscc-observe-devirt.ll<br>
<span class=""><br>
>> The direct answer to your question is #2: it continues running on the now-larger SCC, detects that we switch from one SCC to another at some point, and re-runs on that SCC to make sure that the refined graph is observed.<br>
><br>
> Okay, please add a test case for that.<br>
<br>
</span>This is the same thing, and covered by the same test case. I was just trying to make sure I directly answer your question as well...<br></blockquote><div><br></div><div>Sorry. I think I may have been looking at the earlier version of the patch.</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">
<br>
<br>
> Also add a test case demonstrating that we don't go quadratic on a graph like <a href="http://reviews.llvm.org/F2110388" rel="noreferrer" target="_blank">http://reviews.llvm.org/F2110388</a> where function passes manage to devirtualize all the ref edges.<br>
<br>
I'm not really sure what you want here... In general it is very hard to have a test case in the regression test suite that demonstrates a lack of quadratic behavior -- it typically requires an unacceptably large test case even when the behavior is linear.<br>
<br>
There are also a bunch of things that might "go quadratic" in this case. There are FIXMEs in the code for some of these things that I would like to address, but probably don't belong conflated into this patch...<br>
<br>
Based on the example you post, I think I've figured out that you are trying to point out a case where we will run the SCC pass manager over the function E as many times as we successfully devirtualize edges somewhere in the SCC containing E in a way that brings a new node into that SCC. If I've understood this correctly, then I agree, and that's a nice find. I think it is unlikely to be a problem in practice, but it is definitely something we would need fixed to finish deploying this, probably with just a cap to limit things as a very large SCC formed in this way seems unlikely to be a practical concern to optimize heavily. Given that, I'm inclined to make a FIXME or note about this rather than trying to address it within this patch as that seems like it would bottleneck things.<br>
<br>
Did I understand correctly? Does that approach make sense?<br></blockquote><div><br></div><div>I was thinking about it in slightly different terms (running O(n) times on an SCC of size O(n)) but yes, essentially that is my concern. It will essentially be the same thing as the "max-cg-scc-iterations" limit, right?</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">
<br>
<br>
<a href="http://reviews.llvm.org/D21464" rel="noreferrer" target="_blank">http://reviews.llvm.org/D21464</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>