[PATCH] D24517: GVN-hoist: fix store past load dependence analysis (PR30216)

[Sebastian Pop via llvm-commits]

sebpop added inline comments.

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:347
@@ +346,3 @@
+      // Do not check whether MU aliases Def when MU occurs after OldPt.
+      if (BB == OldBB && firstInBB(OldPt, MU->getMemoryInst()))
+        continue;
----------------
dberlin wrote:
> Rather than repeatedly call firstInBB, can't you just set a boolean when you hit OldPt?
> 
This assumes that "for (const MemoryAccess &MA : *Acc)" iterates through memory accesses in the same order as they appear in the instruction stream.  If this property holds, I could replace the "continue" with a "break": there are no other mem accesses to check after OldPt.

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:351
@@ +350,3 @@
+      // Do not check whether MU aliases Def when MU occurs before NewPt.
+      if (BB == NewBB && firstInBB(MU->getMemoryInst(), NewPt))
+        continue;
----------------
dberlin wrote:
> Ditto. If it's the same BB, the before relationship is entirely controlled by whether you have hit NewPt or not.
Yes, for this one I can use a bool.


https://reviews.llvm.org/D24517