<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 28, 2016 at 1:18 PM, Friedman, Eli <span dir="ltr"><<a href="mailto:efriedma@codeaurora.org" target="_blank">efriedma@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF"><span class="gmail-">
<div class="gmail-m_-4041176510637431261moz-cite-prefix">On 12/28/2016 1:03 PM, Daniel Berlin
via llvm-commits wrote:<br>
</div>
<blockquote type="cite">
<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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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>
</div>
</div>
</blockquote>
<br></span>
I'm not sure what you're trying to say here? PHI nodes for a given
basic block are evaluated simultaneously. From LangRef: "<span>For the purposes
of the SSA form, the use of each incoming value is deemed to occur
on the edge from the corresponding predecessor block to the
current block (but after any definition of an ‘</span><code class="gmail-m_-4041176510637431261docutils gmail-m_-4041176510637431261literal"><span class="gmail-m_-4041176510637431261pre">invoke</span></code><span>‘ instruction’s return value on the same edge)."</span><span class="gmail-HOEnZb"><font color="#888888"><br>
<br></font></span></div></blockquote><div><br></div><div>I'm saying we've had mailing list arguments about this, about whether there is any ordering among phi nodes in a given block. The part you quote from the langref does not actually definitively answer that (again, there is no argument in theory. In the abstract, the answer is "there is no ordering, it's undefined to have phis depend in the same block depend on each other")</div><div><br></div><div>Given</div><div><div>b = phi(d, e)</div><div>a = phi(b, c)</div></div><div><br></div><div>Saying "is deemed to occur on the edge of the corresponding predecessor block" does not help.</div><div><br></div><div>If they are evaluated in the order we see them, it still comes out valid, because one edge will be:<br><br></div><div>b = e</div><div>a = c</div><div><br></div><div>and the other</div><div><br></div><div>b = d</div><div>a = b</div><div><br></div><div>b still dominates all uses.</div><div><br></div><div>However, if they are evaluated simultaneously, then b does dominate a, by the definition of dominance. It's not just a swap copy problem, it's a "this is not well defined code" problem.</div><div><br></div><div>Last i remember, </div><div>1. We never bothered to write down the answer into the langref.</div><div>2. The verifier is fine with bad phis (assuming we do *not* allow the above).</div><div><br></div><div>The following passes verification:<br><div><div>define void @widget() {</div><div>entry:</div><div> br label %bb2</div><div><br></div><div>bb2: ; preds = %bb4, %entry</div><div> %aa = phi i64 [ 5, %entry ], [ %t5, %bb4]</div><div> %t1 = phi i64 [ 0, %entry ], [ %aa, %bb4 ]</div><div> br label %bb4</div><div><br></div><div><br></div><div>bb4: ; preds = %bb3, %bb2</div><div> %t5 = add i64 %t1, 1</div><div> br label %bb2</div><div>}</div></div></div><div><br></div><div>So does:<br><br></div><div><div>define void @widget() {</div><div>entry:</div><div> br label %bb2</div><div><br></div><div>bb2: ; preds = %bb4, %entry</div><div> %t1 = phi i64 [ 0, %entry ], [ %aa, %bb4 ]<br></div><div> %aa = phi i64 [ 5, %entry ], [ %t5, %bb4]</div><div> br label %bb4</div><div><br></div><div><br></div><div>bb4: ; preds = %bb3, %bb2</div><div> %t5 = add i64 %t1, 1</div><div> br label %bb2</div><div>}<br></div></div><div><br></div><div><br></div><div>We've had this discussion before (here was 7 years ago, there is plenty more :P):</div><div><a href="http://lists.llvm.org/pipermail/llvm-dev/2009-January/019779.html">http://lists.llvm.org/pipermail/llvm-dev/2009-January/019779.html</a><br></div><div><br></div><div>Owen is, AFAIK, still correct about "LLVM does not implement this simultaneous evaluation for practical </div><div>reasons. " </div><div>Instead, our phi nodes are effectively executable, instead of just abstract.</div><div><br></div><div>Popping back up, regardless of resolution, this causes the issue i mentioned above - it may require more iterations to resolve because of the second case passing verification. If we really want phi nodes to be executable , and want it to take the minimum number of iterations to converge NewGVN, we need to process aa before t1.</div><div><br></div><div>Otherwise, we will process t1, get some value, *then* process aa, and immediately mark t1 as needing to be reprocessed since it is a use of aa. We effectively waste an iteration because all of t1's uses have are going to have the wrong value.</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div></div></div></div>