<div dir="ltr"><br><div class="gmail_extra">LGTM with those changes.</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 22, 2016 at 10:14 AM, Sebastian Pop <span dir="ltr"><<a href="mailto:sebpop@gmail.com" target="_blank">sebpop@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">sebpop added inline comments.<br>
<span class=""><br>
================<br>
Comment at: llvm/lib/Transforms/Scalar/<wbr>GVNHoist.cpp:347<br>
@@ +346,3 @@<br>
+      // Do not check whether MU aliases Def when MU occurs after OldPt.<br>
+      if (BB == OldBB && firstInBB(OldPt, MU->getMemoryInst()))<br>
+        continue;<br>
----------------<br>
</span><span class="">dberlin wrote:<br>
> Rather than repeatedly call firstInBB, can't you just set a boolean when you hit OldPt?<br>
><br>
</span>This assumes that "for (const MemoryAccess &MA : *Acc)" iterates through memory accesses in the same order as they appear in the instruction stream.</blockquote><div><br></div><div>Yes, it is guaranteed to, and the memoryssa verifiers test the ordering is correct in verifyOrdering.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">  If this property holds, I could replace the "continue" with a "break": there are no other mem accesses to check after OldPt.<br>
<span class=""><br></span></blockquote><div><br>Okay.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
================<br>
Comment at: llvm/lib/Transforms/Scalar/<wbr>GVNHoist.cpp:351<br>
@@ +350,3 @@<br>
+      // Do not check whether MU aliases Def when MU occurs before NewPt.<br>
+      if (BB == NewBB && firstInBB(MU->getMemoryInst(), NewPt))<br>
+        continue;<br>
----------------<br>
</span><span class="">dberlin wrote:<br>
> Ditto. If it's the same BB, the before relationship is entirely controlled by whether you have hit NewPt or not.<br>
</span>Yes, for this one I can use a bool.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D24517" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D24517</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>