<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div style='font-family: arial,helvetica,sans-serif; font-size: 10pt; color: #000000'><br><hr id="zwchr"><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><b>From: </b>"Daniel Berlin" <dberlin@dberlin.org><br><b>To: </b>"Ehsan A Amiri" <amehsan@ca.ibm.com><br><b>Cc: </b>"Hal Finkel" <hfinkel@anl.gov>, "llvm-commits" <llvm-commits@lists.llvm.org>, reviews+D22305+public+92ca108e50bc4651@reviews.llvm.org<br><b>Sent: </b>Friday, July 15, 2016 2:02:37 PM<br><b>Subject: </b>Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming values are the same.<br><br><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 15, 2016 at 11:18 AM, 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: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div><p>Another question for me: With the fix (3) are we going to still have cases that GVN misses for which we need (2) ?</p></div></blockquote><div>No. You should never need #2 because of *GVN*.</div><div><br></div><div>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. Essentially, it's only because of some other bug, that you discovered this one :)</div><div><br></div><div>FWIW: I'm fixing the first GVN bug right now, which will, humorously, hide this bug from regular -O2 because GVN is run twice.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div><p> 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....<br></p></div></blockquote><div><br></div><div id="DWT6806">That is hal's view.</div></div></div></div></blockquote>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.<br><br> -Hal<br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div><p><br>Thanks<br>Ehsan<br><br><br><img src="cid:1__=8FBBF562DFF7DF1D8f9e8a93df938690918c8FB@" alt="Inactive hide details for Daniel Berlin ---07/14/2016 11:08:16 PM---On Thu, Jul 14, 2016 at 7:49 PM, Ehsan A Amiri <amehsan@ca." border="0" height="16" width="16"><font color="#424282">Daniel Berlin ---07/14/2016 11:08:16 PM---On Thu, Jul 14, 2016 at 7:49 PM, Ehsan A Amiri <<a href="mailto:amehsan@ca.ibm.com" target="_blank">amehsan@ca.ibm.com</a>> wrote: > I can measure compile t</font><span class=""><br><br><font color="#5f5f5f" size="2">From: </font><font size="2">Daniel Berlin <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></font><br><font color="#5f5f5f" size="2">To: </font><font size="2">Ehsan A Amiri/Toronto/IBM@IBMCA</font><br><font color="#5f5f5f" size="2">Cc: </font><font size="2">Hal Finkel <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>>, 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></font><br></span><font color="#5f5f5f" size="2">Date: </font><font size="2">07/14/2016 11:08 PM</font></p><div><div class="h5"><br><font color="#5f5f5f" size="2">Subject: </font><font size="2">Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming values are the same.</font><br><hr style="color: rgb(128, 145, 165);" align="left" noshade="noshade" size="2" width="100%"><br><br><br><br><br><font size="4">On Thu, Jul 14, 2016 at 7:49 PM, Ehsan A Amiri <</font><a href="mailto:amehsan@ca.ibm.com" target="_blank"><u><font color="#0000ff" size="4">amehsan@ca.ibm.com</font></u></a><font size="4">> wrote:</font><ul><font size="4">I can measure compile time impact of (2) and (3) on power. A couple of questions/remarks.<br><br>1- As for (2) I don't think there will be a significant compile time cost. </font></ul><br><br><font size="4">Compile time does not get slower 5% at a time, it gets slower 0.1% or 0.05% at a time.</font><br><font size="4"> </font><br><font size="4">It's not "someone adds thing that is super slow", it is "people add lots of little things to lots of functions called lots of times".</font><br><br><font size="4">As Hal points out, we never cache that we simplified or didn't, or checked the phi or didn't, or *anything*, because BasicAA is stateless.</font><br><br><font size="4">Because BasicAA is stateless, we will do this again and again and again (both this phi checking and simplifyinstruction and ....)</font><br><br><font size="4">This adds up.</font><br>
<ul><font size="4">If we do not reduce the phi to its unique incoming value, we will end up in aliasPHI and we do this check there. It is just too late for some cases like the problem here, and we get conservative result. Do I miss something here?</font></ul><br><font size="4">The point is that we are going to try to simplify this, and every phi, a lot of times, both in AA, and elsewhere.</font><br>
<ul><font size="4"><br>2- I will use Daniel's patch for (3). Depending on how expensive it is, we can look at finer grain cache clean ups, as Hal suggests.<br><br>3- I will modify my patch for (2) and modify stripPointerCasts to include "seeing through phi". I wanted to separate that move as a next step, but Hal disagreed on the code review. <br><br>If you disagree with this way of doing the experiments, please let me know.<br><br>Thanks<br>Ehsan<br><br></font><img src="cid:1__=8FBBF562DFF7DF1D8f9e8a93df938690918c8FB@" alt="Inactive hide details for Hal Finkel ---07/14/2016 09:35:07 PM---<hr id=" zwchr=""> > From: "Daniel Berlin" <dberlin@" height="16" width="16"><font color="#424282" size="4">Hal Finkel ---07/14/2016 09:35:07 PM-------- Original Message ----- > From: "Daniel Berlin" <</font><a href="mailto:dberlin@dberlin.org" target="_blank"><u><font color="#0000ff" size="4">dberlin@dberlin.org</font></u></a><font color="#424282" size="4">></font><font size="4"><br></font><font color="#5f5f5f"><br>From: </font>Hal Finkel <<a href="mailto:hfinkel@anl.gov" target="_blank"><u><font color="#0000ff">hfinkel@anl.gov</font></u></a>><font color="#5f5f5f"><br>To: </font>Daniel Berlin <<a href="mailto:dberlin@dberlin.org" target="_blank"><u><font color="#0000ff">dberlin@dberlin.org</font></u></a>><font color="#5f5f5f"><br>Cc: </font>llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank"><u><font color="#0000ff">llvm-commits@lists.llvm.org</font></u></a>>, <<a href="mailto:reviews%2BD22305%2Bpublic%2B92ca108e50bc4651@reviews.llvm.org" target="_blank"><u><font color="#0000ff">reviews+D22305+public+92ca108e50bc4651@reviews.llvm.org</font></u></a>>, Ehsan A Amiri/Toronto/IBM@IBMCA<font color="#5f5f5f"><br>Date: </font>07/14/2016 09:35 PM
<p><font color="#5f5f5f"><br>Subject: </font>Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming values are the same.<font size="4"><br></font></p><hr align="left" noshade="noshade" size="2" width="100%"><font size="4"><br><br></font><font face="Arial" size="4"><br></font><font size="4"><br></font><hr align="left" size="2" width="100%"><b><font face="Arial" size="5"><br>From: </font></b><font face="Arial" size="5">"Daniel Berlin" <</font><a href="mailto:dberlin@dberlin.org" target="_blank"><u><font color="#0000ff" face="Arial" size="5">dberlin@dberlin.org</font></u></a><font face="Arial" size="5">></font><b><font face="Arial" size="5"><br>To: </font></b><font face="Arial" size="5">"Hal Finkel" <</font><a href="mailto:hfinkel@anl.gov" target="_blank"><u><font color="#0000ff" face="Arial" size="5">hfinkel@anl.gov</font></u></a><font face="Arial" size="5">></font><b><font face="Arial" size="5"><br>Cc: </font></b><font face="Arial" size="5">"llvm-commits" <</font><a href="mailto:llvm-commits@lists.llvm.org" target="_blank"><u><font color="#0000ff" face="Arial" size="5">llvm-commits@lists.llvm.org</font></u></a><font face="Arial" size="5">>, </font><a href="mailto:reviews%2BD22305%2Bpublic%2B92ca108e50bc4651@reviews.llvm.org" target="_blank"><u><font color="#0000ff" face="Arial" size="5">reviews+D22305+public+92ca108e50bc4651@reviews.llvm.org</font></u></a><font face="Arial" size="5">, "Ehsan A Amiri" <</font><a href="mailto:amehsan@ca.ibm.com" target="_blank"><u><font color="#0000ff" face="Arial" size="5">amehsan@ca.ibm.com</font></u></a><font face="Arial" size="5">></font><b><font face="Arial" size="5"><br>Sent: </font></b><font face="Arial" size="5">Thursday, July 14, 2016 8:30:09 PM</font><b><font face="Arial" size="5"><br>Subject: </font></b><font face="Arial" size="5">Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming values are the same.</font><ul><ul><font face="Arial" size="4"><br>(2) is, in theory, the right thing to do. Even if we were to consider uniform PHIs to be anti-canonical, and thus something which should be simplified, we can't simplify often enough to prevent these from blocking useful analysis work. </font></ul></ul><font face="Arial" size="5"><br>FWIW, i'm fine with this approach if our approach is going to be as you say - that we will not simplify often enough.</font><font size="4"><br></font><font face="Arial" size="5"><br>Right now, as i said, we simplify *everywhere*, and every one of those calls will eliminate this phi node.</font><font size="4"><br></font><font face="Arial" size="5"><br>So it's only *this particular path* that misses all those calls.</font><font size="4"><br></font><font face="Arial" size="5"><br>For example, if the alias check had gone through a gep of a phi anywhere, it would simplify the phi as part of getunderlyingobject, etc.</font><ul><ul><font face="Arial" size="4">Arbitrary uses of RAUW can create these PHIs, and we can't (and probably shouldn't) run InstCombine in between every other pass. This is a local pattern that stripPointerCasts, and similar functions, can look through.</font></ul></ul><font face="Arial" size="5"><br>Fine with this as long as we maybe stop trying to simplify instructions 8 or 9 times, and instead do it once (max) per instructions, and make this part of it.</font><font size="4"><br></font><font face="Arial" size="5"><br>(IE This would mean we would have SimplifyAndGetUnderlyingObject and GetUnderlyingObject, and we simplify once and call the latter or something, or whatever. Not suggesting we decide this second, just suggesting that we agree if this is going to be our general approach).</font><font face="Arial" size="4"><br>This is hard because we don't cache the simplifications in any way. It is not like we're updating the IR when we discover some simplification; we're only using the simplified version in place. I'm not sure how to fix this. Maybe we should run InstSimplify a lot more often. It is not as expensive was InstCombine by a large margin (IIRC).</font><ul><ul><font face="Arial" size="4"><br>(3) is also, in theory, the right thing to do. The memdep cache, by necessity, caches negative results. Each GVN iteration, however, performs "information revealing" operations which can make the cached results more conservative than a new query's results.</font></ul></ul><font face="Arial" size="5"><br>Yes.<br>It's theoretically possible to make it less expensive than blowing away the whole cache, but so far, experience has told me that fully maintaining the cache becomes slower than redoing the queries :P</font><font face="Arial" size="4"><br>I'm not sure how much this helps, but we could/should only clear out the MayAlias results.<br><br>-Hal</font><ul><ul><font face="Arial" size="4"><br>Now we actually need to measure the costs.<br><br>-Hal</font><font size="4"><br><br></font><font face="Arial" size="5"><br>(Note: i've attached a patch for 3 in case anyone wants to see the compile time cost)</font><font size="4"><br><br><br><br><br><br><br></font><font face="Arial" size="5"><br>On Thu, Jul 14, 2016 at 2:12 PM, Daniel Berlin <</font><a href="mailto:dberlin@dberlin.org" target="_blank"><u><font color="#0000ff" face="Arial" size="5">dberlin@dberlin.org</font></u></a><font face="Arial" size="5">> wrote:<br>Note: This already had GVN run once on it, do you have the one before that?</font><font size="4"><br><br></font><font face="Arial" size="5"><br>On Thu, Jul 14, 2016 at 1:53 PM, Ehsan A Amiri <</font><a href="mailto:amehsan@ca.ibm.com" target="_blank"><u><font color="#0000ff" face="Arial" size="5">amehsan@ca.ibm.com</font></u></a><font face="Arial" size="5">> wrote:</font><ul><ul><font face="Arial" size="5">I can see the problem with the following command line and attached file <br><br>opt -gvn t.ll</font><i><font face="Arial" size="5"><br><br>(See attached file: t.ll)</font></i><font face="Arial" size="5"><br><br>My clang is almost a week old.<br></font><font size="4"><br></font><img src="cid:1__=8FBBF562DFF7DF1D8f9e8a93df938690918c8FB@" alt="Inactive hide details for Daniel Berlin ---07/14/2016 04:27:43 PM---Actually, can you please attach a .ll file and an opt comma" height="16" width="16"><font color="#424282" face="Arial" size="5">Daniel Berlin ---07/14/2016 04:27:43 PM---Actually, can you please attach a .ll file and an opt command line that reproduces the problem?</font><font color="#5f5f5f" face="Arial" size="4"><br><br>From: </font><font face="Arial" size="4">Daniel Berlin <</font><a href="mailto:dberlin@dberlin.org" target="_blank"><u><font color="#0000ff" face="Arial" size="4">dberlin@dberlin.org</font></u></a><font face="Arial" size="4">></font><font color="#5f5f5f" face="Arial" size="4"><br>To: </font><font face="Arial" size="4">Ehsan A Amiri/Toronto/IBM@IBMCA</font><font color="#5f5f5f" face="Arial" size="4"><br>Cc: </font><font face="Arial" size="4">Hal Finkel <</font><a href="mailto:hfinkel@anl.gov" target="_blank"><u><font color="#0000ff" face="Arial" size="4">hfinkel@anl.gov</font></u></a><font face="Arial" size="4">>, llvm-commits <</font><a href="mailto:llvm-commits@lists.llvm.org" target="_blank"><u><font color="#0000ff" face="Arial" size="4">llvm-commits@lists.llvm.org</font></u></a><font face="Arial" size="4">>, </font><a href="mailto:reviews%2BD22305%2Bpublic%2B92ca108e50bc4651@reviews.llvm.org" target="_blank"><u><font color="#0000ff" face="Arial" size="4">reviews+D22305+public+92ca108e50bc4651@reviews.llvm.org</font></u></a><font color="#5f5f5f" face="Arial" size="4"><br>Date: </font><font face="Arial" size="4">07/14/2016 04:27 PM</font><font color="#5f5f5f" face="Arial" size="4"><br>Subject: </font><font face="Arial" size="4">Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming values are the same.</font><font size="4"><br></font><hr align="left" noshade="noshade" size="2" width="100%"><font face="Arial" size="5"><br></font><font size="4"><br></font><font face="Arial" size="6"><br>Actually, can you please attach a .ll file and an opt command line that reproduces the problem?<br>The clang command line you have is very sensitive to versions, etc.<br><br>I cannot get your issue to reproduce with the clang i have installed that can target powerpc-linux (and the issue does not reproduce with your testcase on x86) :)<br><br>While debugging a bit, note that there is at least one obvious bug in GVN that may affect this, by inspection:<br><br>When GVN splits a critical edge, it never adds the new block to the iteration order (at all), even though it inserts into it.<br>So they will not get processed until the next iteration of GVN on the function, even though they have code in them. While this is okay from a correctness standpoint, it may block optimization of certain things (including the cases you've discovered). In practice, there is no way to perfectly solve that without pre-splitting all critical edges, but you should get the same effect if we throw the critical edge block and then it's successors (including the current blcok) into bbvect after the current block again.<br><br>It is also missing a real phi simplification.<br>While simplifyinstruction will check if all arguments are trivially the same, that is not the real test that should be performed.<br><br>It should be doing VN.lookup on each argument and seeing if they come up with the the same value number.<br><br>Once you attach the .ll file, i'll fix these and see if it fixes your testcase, and if not, debug further.</font><font face="Arial" size="5"><br></font><font face="Arial" size="6"><br><br>On Thu, Jul 14, 2016 at 10:11 AM, Daniel Berlin <</font><a href="mailto:dberlin@dberlin.org" target="_blank"><u><font color="#0000ff" face="Arial" size="6">dberlin@dberlin.org</font></u></a><font face="Arial" size="6">> wrote:</font><ul><ul><ul><ul><font face="Arial" size="6"><br><br>On Thu, Jul 14, 2016 at 7:09 AM, Ehsan A Amiri <</font><a href="mailto:amehsan@ca.ibm.com" target="_blank"><u><font color="#0000ff" face="Arial" size="6">amehsan@ca.ibm.com</font></u></a><font face="Arial" size="6">> wrote:<br>This is order of events:<br>This order cannot be correct if the solution I gave (or adding simplification to *some place in GVN*) does not work. Or GVN is broken in other ways.<br><br>1) GVN starts looking at the function. At this point the phi node has two different incoming values.<br>2) GVN performs an RAUW. The phi is converted to the one that has two identincal incoming values. <br><br>At this point, it should now process the phi instruction again before it processes the load, because it is doing a reverse postorder traversal.<br>When it did that, the phi should have been simplified<br>So why did that not happen?<br><br>Given the complexity of fixing the real problem,<br><br>Look, i understand why you want to just fix this in AA and be done with it.<br>Really, I do. <br><br>I understand you have spent a lot of time on this bug, and I greatly appreciate that.<br>But I really want to understand what is going on before we try to actually fix it.<br>I have a good understanding of what happens once the bad answer gets into memdep (and thank you for that!), but i still have trouble seeing why it lived long enough to get there.<br><br>To that end, so you don't have to spend more time running around for me, i'll take over this bug, and either figure out why GVN lets this PHI live to the point it gets an AA query about it (and fix it/decide it can't be fixed), or commit the AA patch for you if we decide it can't be fixed.<br><br>My ETA is by friday.<br><br>I assume the testcase in the bug is the one we are still using, right?<br>(If not, if you can attach it, that would be helpful)</font></ul></ul></ul></ul></ul></ul><font size="4"><br><br></font><font face="Arial" size="4"><br><br></font><font size="4"><br></font><font color="#888888" face="Arial" size="4"><br>-- <br>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory</font></ul></ul><font face="Arial" size="4"><br><br><br><br>-- <br>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory</font><br><font size="4">[attachment "graycol.gif" deleted by Ehsan A Amiri/Toronto/IBM] <br></font><p></p></ul><br>
<p><br>
</p></div></div><p></p></div>
</blockquote></div><br></div></div>
</blockquote><br><br><br>-- <br><div><span name="x"></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span name="x"></span><br></div></div></body></html>