<div dir="ltr">Danny's right; the "we need to cache everything immediately" behavior doesn't play very nicely with loops.<div><br></div><div>The new walker (which should be out for review later today) correctly optimizes %loadG1_1 and %loadG1_2 to liveOnEntry. :)</div><div><br></div><div>George</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 27, 2016 at 11:14 AM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>This is definitely a caching bug related to this code:</div><div>   1038    // Don't try to optimize this phi again if we've already tried to do so.</div><div><div>   1039    if (!Q.Visited.insert(PHIPair).second) {</div><div>   1040<span style="white-space:pre-wrap">     </span>      ModifyingAccess = CurrAccess;</div><div>   1041<span style="white-space:pre-wrap">  </span>      break;</div><div>   1042<span style="white-space:pre-wrap"> </span>    }</div></div><div><br></div><div><br></div><div>We don't differentiate elsewhere between having stopped at a phi because we were path walking and discovered that path has no conflicts, and having stopped at a phi because that was the clobbering access.</div><div><br></div><div>Because of this, we cache the result as if it was a clobbering access, when it really means "you can ignore this path".</div><div><br></div><div>The logic in the code that handles the former case only exists after the cache lookups, and so the first use properly detects that it can ignore this phi, and the second use , which just hits the cache, cannot.</div><div><br></div><div><br></div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 27, 2016 at 10:27 AM, Geoff Berry <span dir="ltr"><<a href="mailto:gberry@codeaurora.org" target="_blank">gberry@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hey All,<br>
<br>
I've come across what I believe to be a bug in MemorySSA. George, I wasn't sure if this was a known issue that you'll be addressing in your upcoming walker caching changes or not, so I haven't investigated it very much.  The test case is attached. The bug is that the defining access for the second load is set to the loop MemoryPhi node instead of being liveOnEntry as it should be as I understand things.<span><font color="#888888"><br>
<br>
-- <br>
Geoff Berry<br>
Employee of Qualcomm Innovation Center, Inc.<br>
 Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project<br>
<br>
</font></span></blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>