<html><body><p>This is order of events:<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>3) GVN makes a mem-dep query for dependencies of a load instruction. (Let's denote it with A)<br>4) memdep encounters a store insn (let's denote it with B). <br>5) memdep performs an alias analysis query for pointer operands of A and B.<br>6) memdep is told that the pointer operands of A and B are aliased. (They are not really aliased, but BasicAA can not see through the phi node)<br>7) memdep now thinks that B clobbers A. This information is cached.<br>8) GVN is told that B clobbers A. GVN gives up on removing A.<br>9) GVN removes the phi node.<br>10) GVN tries again to remove A. It queries memdep. <br>11) memdep looks at its cache. It finds that B clobbers A. It returns that information to GVN.<br>12) GVN gives up again and does not touch A.<br><br>Now if mem-dep cache was working properly,in step 11 we would have made a new alias analysis and then prove that B does not clobber A and A would have been removed in step 12. <br><br>Given the complexity of fixing the real problem, if alias analysis can see through phi, we can prove that pointer operands of A and B are not aliased in steps 5 and 6, so memdep never returns or cache a conservative result.<br><br>Thanks<br>Ehsan<br> <br><br><img width="16" height="16" src="cid:1__=8FBBF563DFDF0B768f9e8a93df938690918c8FB@" border="0" alt="Inactive hide details for Daniel Berlin ---07/13/2016 11:57:11 PM---So now I'm even more confused. If it's using the memdep cac"><font color="#424282">Daniel Berlin ---07/13/2016 11:57:11 PM---So now I'm even more confused. If it's using the memdep cache, why does changing basicaa work at all</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/13/2016 11:57 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">So now I'm even more confused. If it's using the memdep cache, why does changing basicaa work at all?</font><br><font size="4">Should it not still get the cached info<br></font><br><font size="4">On Wed, Jul 13, 2016, 8:22 PM Ehsan A Amiri <</font><a href="mailto:amehsan@ca.ibm.com"><u><font size="4" color="#0000FF">amehsan@ca.ibm.com</font></u></a><font size="4">> wrote:</font><ul><font size="4">Daniel,<br><br>The soultion you are proposing does not work. Actually it is already implemented and we have this problem. Very early in processInstruction, there is a call to SimplifyInstruction, which provides that exact same functionality for PHI nodes and we still have this problem.<br><br>This goes back to what we already discussed in detail in the PR. Actually GVN does not directly make an Alias Analysis query. It makes a MemDep query. At the first iteration of GVN, when thet mem-dep does not have the results cached (and so it does all the work, which includes alias analysis query), the phi exists. After the phi is removed, the cache already contains invalid information, so memdep does not make a new alias analysis query. It just provides us with its invalid cache information. So we are back at our previous discussion :)<br><br>Thanks<br>Ehsan <br><br></font><img src="cid:1__=8FBBF563DFDF0B768f9e8a93df938690918c8FB@" width="16" height="16" alt="Inactive hide details for Daniel Berlin ---07/13/2016 09:34:56 PM---On Wed, Jul 13, 2016 at 5:55 PM, Ehsan Amiri <amehsan@ca.ib"><font size="4" color="#424282">Daniel Berlin ---07/13/2016 09:34:56 PM---On Wed, Jul 13, 2016 at 5:55 PM, Ehsan 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" color="#424282">> wrote: > amehsan added a comment.</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><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>Cc: </font>Ehsan A Amiri/Toronto/IBM@IBMCA, 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>><font color="#5F5F5F"><br>Date: </font>07/13/2016 09:34 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><p><font size="4"><br><br><br><br></font><font size="5"><br>On Wed, Jul 13, 2016 at 5:55 PM, Ehsan 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:</font><ul><ul><font size="5">amehsan added a comment.<br><br>@dberlin<br><br>I see two concerns in your comment:<br><br>> > Why not fix GVN to do that?<br><br>><br><br>><br><br>><br><br><br>Before GVN this phi node has two different incoming values. At some point A in GVN, one of the incoming values change and we get the phi node in the example above. At some point B in the GVN, the phi node is removed. Somewhere between A and B, GVN makes an alias analysis query. So I think there is not really a problem in GVN to fix.</font></ul></ul><font size="5"><br>I'm going to pretty strongly disagree. If GVN expects AA to give it good intermediate answers, it needs to give AA good intermediate code.</font><font size="4"><br></font><font size="5"><br>GVN does a lot of simplification already. In fact, this would probably be one of the cheapest things GVN does It seems 100% trivial to make this work, unless you can show me it won't :)</font><font size="4"><br></font><font size="5"><br>Change:</font><font size="4"><br></font><font size="5"><br> for (BasicBlock::iterator BI = BB->begin(), BE = BB->end();<br> BI != BE;) {<br> if (!ReplaceWithConstMap.empty())<br> ChangedFunction |= replaceOperandsWithConsts(&*BI);<br> ChangedFunction |= processInstruction(&*BI);</font><font size="4"><br></font><font size="5"><br>To have a processPhiNode call at the beginning.<br>In processPhiNode, if all arguments are the same, RAUW with the phi node result.</font><font size="4"><br></font><font size="5"><br>Or heck, throw it in processInstruction</font><font size="4"><br></font><font size="5"><br>Now, if that doesn't work, or you have actual inter-pass examples where this is getting left around, sure, fine.<br>I want to avoid useless checks in alias analysis (and elsewhere) for non-canonical code. </font><font size="4"><br></font><font size="5"><br>Look at it another way:</font><font size="4"><br></font><font size="5"><br>If this is common IR, every analysis is likely to need to do something with it, at some cost.</font><font size="4"><br></font><font size="5"><br>It's much cheaper to *not* have to process it at all, than have to process it everywhere.</font><font size="4"><br><br><br><br></font><font size="5"><br> </font><font size="4"><br></font><br></ul><BR>
</body></html>