<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 15, 2016 at 1:34 PM, Ehsan A Amiri <span dir="ltr"><<a href="mailto:amehsan@ca.ibm.com" target="_blank">amehsan@ca.ibm.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><p>So I will first experiment with (3). If we don't like the outcome, then we can experiment with (2) to see how feasible is that.<br><br><b>Daniel</b>: Thanks for the response to my question.  But now I need to ask two more question to understand things:<span class=""><br><br><i><font size="4">As mentioned, in your testcase, there is another bug here.  Remember  that GVN iterates until things stop changing. So *the first gvn pass* should have never left the IR with that redundant PHI node in the first place.</font></i><br><br></span><font size="4">1- IIUC, we are saying that at the end of one GVN iteration, the IR should satisfy some properties. Is it possible to describe this properties? </font></p></div></blockquote><div><br></div><div>Close. I mean "at the end of executing a  GVN pass, the IR should satisfy some properties" (iterations are internal to gvn, and in the current GVN, it may take multiple iterations to satisfy these properties).</div><div><br></div><div>The properties it should satisfy are[1]</div><div><br></div><div>1. Every value (V) dominated by another provably equivalent value (Leader) should have V replaced by Leader.  This is "equivalence without taking into account conditionals", and not even complete equivalence, but "equivalence that this GVN algorithm can detect".  The GVN algorithm used is not a complete one, so it misses plenty of provably equivalences.</div><div><br></div><div>As a trivial example:</div><div><br></div><div>for (int i =0, j = 0; i < 50; ++i, ++j)</div><div>{</div><div>int x = i + 2;</div><div>int y = j + 2;</div><div>A[x] = y</div><div>int z = A[y];</div><div>}<br></div><div><br></div><div>i, j, x, y, z are all equivalent.</div><div>Current GVN will detect none of these. This is not fixable.</div><div><br></div><div>NewGVN is not complete, but it will detect all of these.</div><div>Complete algorithms will detect all that can be detected :)</div><div><br></div><div>2. Every instruction should be simplified as done by SimplifyInst. If simplification produces a value, all uses should be replaced with that simplified value</div><div><br></div><div>[1] There are PRE and conditional propagation properties, but they are harder to describe.<br></div><div><br></div><div>Now, for *each iteration*, there are invariants. The above is an iteration level invariant except for the case where it does PRE and inserts code.  In the case it does PRE and inserts code, it may not meet the above conditions.</div><div><br></div><div><br></div><div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><p><br><font size="4">2- I suspect that the "other bug" that you are talking about is a GVN bug. Could you elaborate on that if this is correct?</font></p></div></blockquote><div>Yes.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><p><font size="4"> IIRC from my debugging, the redundant phi node was created and deleted in iteration 0 of GVN.</font></p></div></blockquote><div> I do not see this happen in some cases where i modify your testcase.</div><div>Again i was not being precise enough, and meant more "things your testcase exposes after debugging the code".<br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><p><font size="4"> It was just that cache issue that happened between the creation and deletion of redundant phi node that caused this....</font><br><br><font size="4">Thanks</font><br><font size="4">Ehsan</font><br><br><br><br><img width="16" height="16" src="cid:1__=8FBBF562DFFC7FAB8f9e8a93df938690918c8FB@" border="0" alt="Inactive hide details for Daniel Berlin ---07/15/2016 04:08:45 PM---> >"><font color="#424282">Daniel Berlin ---07/15/2016 04:08:45 PM---> ></font><span class=""><br><br><font size="2" color="#5F5F5F">From:        </font><font size="2">Daniel Berlin <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></font><br><font size="2" color="#5F5F5F">To:        </font><font size="2">Hal Finkel <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></font><br></span><font size="2" color="#5F5F5F">Cc:        </font><font size="2">llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>>, <a href="mailto:reviews%2BD22305%2Bpublic%2B92ca108e50bc4651@reviews.llvm.org" target="_blank">reviews+D22305+public+92ca108e50bc4651@reviews.llvm.org</a>, Ehsan A Amiri/Toronto/IBM@IBMCA</font><br><font size="2" color="#5F5F5F">Date:        </font><font size="2">07/15/2016 04:08 PM</font><span class=""><br><font size="2" color="#5F5F5F">Subject:        </font><font size="2">Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming values are the same.</font><br></span></p><hr width="100%" size="2" align="left" noshade style="color:#8091a5"><br><br>
<ul><br><div><div class="h5"><br><font size="4" face="Arial">Or do we need (2) because in general this kind of phi node is seen in many programs? Or something else that I do not see....</font><p><br><font size="4" face="Arial">That is hal's view.</font></p></div></div></ul><div><div class="h5">
<ul><font face="Arial">My view was that we should measure the impact of the change and then decide; but that if we have a use case for it, I'd rather it be inside the generic utility than a specific work-around in BasicAA.</font></ul><br><font size="4">Yes, sorry, was writing quickly, did not mean to imply anything else ;)</font><br><br><br>
</div></div><p></p></div>
</blockquote></div><br></div></div>