<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 28, 2016 at 7:04 AM, Davide Italiano via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">davide accepted this revision.<br>
davide added a comment.<br>
This revision is now accepted and ready to land.<br>
<br>
Sorry for the slow response, I'm out('ish) of the office these days. I took a close look at your patch.<br></blockquote><div><br></div><div>No worries.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I happen to be lucky enough to hit a case in the wild where this already matters.  The number of iteration goes down from hundreds to ~10, which makes compile time/me happier.<br></blockquote><div><br></div><div>yay.</div><div><br></div><div>The current code, excepting super-weird cases, should operate in O(d+3) iterations, where d is the loop connectedness of the SSA graph (not the CFG), which is the number of backedges in any path. This will change when we move to equality propagation, but for now, ...</div><div>We could  calculate this number and see if we are screwing up :)<br><br></div><div>For most programs, the loop connectedness of the SSA graph is the same or less than the CFG.</div><div><br></div><div>However,  IIRC, we allow  dependent phis in the same block (this is not strictly SSA, since all phi nodes are supposed to be evaluated simultaneously).</div><div><br></div><div>If that is correct, you can construct programs where the loop connectedness of the SSA graph is much worse than the CFG.</div><div><br></div><div><br></div><div>IE</div><div>f = phi(h, i)</div><div>d = phi(f, g)</div><div>b = phi(d, e)</div><div>a = phi(b, c)</div><div> <br></div><div>in the same block.</div><div><br></div><div><br></div><div>We can resolve this issue a number of ways, like ordering the phi nodes in the block topologically, finding and iterating the SCC's specially, blah blah blah.</div><div><br></div><div>There are also other tricks we can play to speed convergence - much like SCCP, it's an optimistic algorithm, so we start by assuming everything is the same.  This means the first iteration should be maximal (though maybe not correct).</div><div>Anything that we don't find a congruence for (IE end up in their own class) on the first iteration, we will never find a congruence for, and there is no point in revisiting them, no matter what else changes, because our answer can't get better, only worse.</div><div>At least, i believe this is correct, i'll prove it one way or the other.</div><div><br></div><div>Note that this is one difference between the branch predicate handling and the paper predicate handling - the branch predicate handling can cause significantly more iterations. The paper predicate handling not so much.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Your reasoning looks good to me, so feel free to check this in (modulo small nits). I'll do some more extensive testing when I'm in back in California early next year (although as you pointed out this is not exposed without predicates handling.<br>
<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/NewGVN.c<wbr>pp:1375-1377<br>
<span>+  // The dominator tree does guarantee that, for a given dom tree node, it's<br>
+  // parent must occur before it in the RPO ordering. Thus, we only need to sort<br>
+  // the siblings.<br>
</span>----------------<br>
I apologize in advance if I'm wrong, as I'm not a native speaker, but it shouldn't be<br>
`its parent` (at least this is the way I've always seen it written down)?<br>
Please ignore otherwise.<br></blockquote><div><br>Yes it should be its.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/NewGVN.c<wbr>pp:1457<br>
   }<br>
-<br>
   Changed |= eliminateInstructions(F);<br>
----------------<br>
Spurious removal of a newline?<br>
<br>
<br></blockquote><div>Fixed!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<a href="https://reviews.llvm.org/D28129" rel="noreferrer" target="_blank">https://reviews.llvm.org/D2812<wbr>9</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>