<html><body><p><i>(See attached file: t.ll)</i><br>This one should be correct. (Previous one seems to be something from the middle of GVN :P )<br><br><img width="16" height="16" src="cid:2__=8FBBF563DFE595048f9e8a93df938690918c8FB@" border="0" alt="Inactive hide details for Daniel Berlin ---07/14/2016 05:12:47 PM---Note: This already had GVN run once on it, do you have the "><font color="#424282">Daniel Berlin ---07/14/2016 05:12:47 PM---Note: This already had GVN run once on it, do you have the one before that? On Thu, Jul 14, 2016 at</font><br><br><font size="2" color="#5F5F5F">From: </font><font size="2">Daniel Berlin <dberlin@dberlin.org></font><br><font size="2" color="#5F5F5F">To: </font><font size="2">Ehsan A Amiri/Toronto/IBM@IBMCA</font><br><font size="2" color="#5F5F5F">Cc: </font><font size="2">Hal Finkel <hfinkel@anl.gov>, llvm-commits <llvm-commits@lists.llvm.org>, reviews+D22305+public+92ca108e50bc4651@reviews.llvm.org</font><br><font size="2" color="#5F5F5F">Date: </font><font size="2">07/14/2016 05:12 PM</font><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><hr width="100%" size="2" align="left" noshade style="color:#8091A5; "><br><br><br><font size="4">Note: This already had GVN run once on it, do you have the one before that?<br></font><br><br><font size="4">On Thu, Jul 14, 2016 at 1:53 PM, Ehsan A Amiri <</font><a href="mailto:amehsan@ca.ibm.com" target="_blank"><u><font size="4" color="#0000FF">amehsan@ca.ibm.com</font></u></a><font size="4">> wrote:</font><ul><font size="4">I can see the problem with the following command line and attached file <br><br>opt -gvn t.ll<br></font><i><font size="4"><br>(See attached file: t.ll)</font></i><font size="4"><br><br>My clang is almost a week old.<br><br></font><img src="cid:2__=8FBBF563DFE595048f9e8a93df938690918c8FB@" width="16" height="16" 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"><font size="4" color="#424282">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 size="4"><br></font><font color="#5F5F5F"><br>From: </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>To: </font>Ehsan A Amiri/Toronto/IBM@IBMCA<font color="#5F5F5F"><br>Cc: </font>Hal Finkel <<a href="mailto:hfinkel@anl.gov" target="_blank"><u><font color="#0000FF">hfinkel@anl.gov</font></u></a>>, 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><font color="#5F5F5F"><br>Date: </font>07/14/2016 04:27 PM<font color="#5F5F5F"><br>Subject: </font>Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming values are the same.<br><hr width="100%" size="2" align="left" noshade><font size="4"><br><br></font><br><font size="5">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.</font><font size="4"><br></font><font size="5"><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) :)</font><font size="4"><br></font><font size="5"><br>While debugging a bit, note that there is at least one obvious bug in GVN that may affect this, by inspection:</font><font size="4"><br></font><font size="5"><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.</font><font size="4"><br></font><font size="5"><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.</font><font size="4"><br></font><font size="5"><br>It should be doing VN.lookup on each argument and seeing if they come up with the the same value number.</font><font size="4"><br></font><font size="5"><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 size="4"><br><br></font><font size="5"><br>On Thu, Jul 14, 2016 at 10:11 AM, Daniel Berlin <</font><a href="mailto:dberlin@dberlin.org" target="_blank"><u><font size="5" color="#0000FF">dberlin@dberlin.org</font></u></a><font size="5">> wrote:</font><ul><ul><font size="4"><br></font><font size="5"><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 size="5" color="#0000FF">amehsan@ca.ibm.com</font></u></a><font size="5">> wrote:<br>This is order of events:</font><p><font size="5">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. </font><p><font size="5"><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?</font><font size="4"><br></font><font size="5"><br>Given the complexity of fixing the real problem,</font><p><font size="5"><br>Look, i understand why you want to just fix this in AA and be done with it.<br>Really, I do. </font><font size="4"><br></font><font size="5"><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.</font><font size="4"><br></font><font size="5"><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.</font><font size="4"><br></font><font size="5"><br>My ETA is by friday.</font><font size="4"><br></font><font size="5"><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><font size="4"><br></font></ul></ul><font size="4"><br></font></ul><br>
<p><BR>
</body></html>