So now I'm even more confused. If it's using the memdep cache, why does changing basicaa work at all?<div>Should it not still get the cached info<br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jul 13, 2016, 8:22 PM Ehsan A Amiri <<a href="mailto:amehsan@ca.ibm.com">amehsan@ca.ibm.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex" id="gmail_block_quote0"><div><p>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><img width="16" height="16" src="cid:155e78b2e15308fca931" border="0" 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" id="i155e78b2e15308fca931" class="nativeView" style="width: 329px;"><font color="#424282">Daniel Berlin ---07/13/2016 09:34:56 PM---On Wed, Jul 13, 2016 at 5:55 PM, Ehsan Amiri <<a href="mailto:amehsan@ca.ibm.com" target="_blank">amehsan@ca.ibm.com</a>> wrote: > amehsan added a comment.</font><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"><a href="mailto:reviews%2BD22305%2Bpublic%2B92ca108e50bc4651@reviews.llvm.org" target="_blank">reviews+D22305+public+92ca108e50bc4651@reviews.llvm.org</a></font><br><font size="2" color="#5F5F5F">Cc:        </font><font size="2">Ehsan A Amiri/Toronto/IBM@IBMCA, 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>></font><br><font size="2" color="#5F5F5F">Date:        </font><font size="2">07/13/2016 09:34 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></p><hr width="100%" size="2" align="left" noshade style="color:#8091a5"><p></p></div><div><p><br><br><br><br><br><font size="4">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">> wrote:</font></p><ul><font size="4">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><br><font size="4">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><br><br><font size="4">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><br><br><font size="4">Change:</font><br><br><font size="4">   for (BasicBlock::iterator BI = BB->begin(), BE = BB->end();</font><br><font size="4">        BI != BE;) {</font><br><font size="4">     if (!ReplaceWithConstMap.empty())</font><br><font size="4">       ChangedFunction |= replaceOperandsWithConsts(&*BI);</font><br><font size="4">     ChangedFunction |= processInstruction(&*BI);</font><br><br><font size="4">To have a processPhiNode call at the beginning.</font><br><font size="4">In processPhiNode, if all arguments are the same, RAUW with the phi node result.</font><br><br><font size="4">Or heck, throw it in processInstruction</font><br><br><font size="4">Now, if that doesn't work, or you have actual inter-pass examples where this is getting left around, sure, fine.</font><br><font size="4">I want to avoid useless checks in alias analysis (and elsewhere) for non-canonical code.  </font><br><br><font size="4">Look at it another way:<br></font><br><font size="4">If this is common IR, every analysis is likely to need to do something with it, at some cost.</font><br><br><font size="4">It's much cheaper to *not* have to process it at all, than have to process it everywhere.</font><br><br><br><br><br><font size="4"> </font><br><br>
<p></p></div></blockquote></div></div>