[PATCH] D8688: Update MergedLoadStoreMotion to use MemorySSA

Geoff Berry via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 6 11:39:34 PDT 2016

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?


More information about the llvm-commits mailing list