[PATCH] D8688: Update MergedLoadStoreMotion to use MemorySSA

[Geoff Berry via llvm-commits]

gberry added a comment.

A few more comments/questions below.  I still have a few parts I haven't fully grasped yet, but don't let me hold up approval.


================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:93
@@ -91,1 +92,3 @@
+#include <unordered_set>
+#include <vector>
 
----------------
vector doesn't appear to be used?

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:158
@@ +157,3 @@
+  // successors.
+  DenseMap<const Instruction *, unsigned> DFSNumberMap;
+  DenseMap<const BasicBlock *, unsigned> FirstHoistBarrier;
----------------
Can't you just compute these instruction indices for the diamond then/else blocks lazily?  The same goes for LastSinkBarrier, which wouldn't even need a map in that case.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:771
@@ +770,3 @@
+  // This isUsedOutsideOfBlock is also conservative.
+  if (!Load1 || Load1->isUsedOutsideOfBlock(BB))
+    return false;
----------------
Maybe check Load1->isSimple() here?  Maybe factor out all these checks into a function and use it for the Load0 hash table insertion as well?

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:799
@@ +798,3 @@
+
+  while (!Res && LookupIter != LookupResult.second) {
+    LoadInst *Load0 = LookupIter->first;
----------------
Isn't this still O(M^2) if e.g. all of the loads have the same clobbering access and the same types? I guess the limit above controls how big M can get though.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:997
@@ +996,3 @@
+  // Seed with current users that are in the block range.
+  std::queue<MemoryAccess *> CurrentUses;
+  SmallPtrSet<const MemoryAccess *, 8> Visited;
----------------
Is there a reason you're using a queue for this work-list instead of the more common SmallVector?


http://reviews.llvm.org/D8688