[PATCH] D8688: Update MergedLoadStoreMotion to use MemorySSA

[Chad Rosier via llvm-commits]

mcrosier added a comment.

A few comments in passing.


================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:183
@@ -116,1 +182,3 @@
     AU.setPreservesCFG();
+    AU.addRequired<TargetLibraryInfoWrapperPass>();
+    AU.addRequired<DominatorTreeWrapperPass>();
----------------
Perhaps I missed something, but why is TargetLibraryInfo required?

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:189
@@ -119,2 +188,3 @@
+    AU.addPreserved<MemorySSAWrapperPass>();
     AU.addPreserved<MemoryDependenceWrapperPass>();
   }
----------------
Should the DominatorTree be added as preserved?

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:577
@@ -482,1 +576,3 @@
     assert(S1->getParent() == A1->getParent());
+    if (UseMemorySSA) {
+      updateMemorySSAForSunkStores(BB, S0, S1, SNew);
----------------
No need for extra curly brackets.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:97
@@ -93,1 +96,3 @@
 #define DEBUG_TYPE "mldst-motion"
+static cl::opt<bool> UseMemorySSA("use-memoryssa", cl::Hidden,
+                                  cl::desc("Use MemorySSA for optimizations"));
----------------
Should we try and unique the name a bit more (e.g., "use-memssa-mlsm") under the assumption that other passes will have a command-line option to opt into using MemorySSA?  Alternatively, we could include the option in the PassManagerBuilder and use this single flag to control the use of MemorySSA for all passes during the transition period. Just my 2c.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:747
@@ +746,3 @@
+
+  // Walk all accesses in first block, but them into a hash table.
+  for (auto &Access : *Succ0Accesses) {
----------------
but -> put

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:762
@@ +761,3 @@
+    ++NLoads;
+    if (NLoads * Size1 >= MagicCompileTimeControl)
+      break;
----------------
Pre-increment here?

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:769
@@ +768,3 @@
+    auto CurrIter = AI;
+    ++AI;
+    const MemoryUse *MU = dyn_cast<MemoryUse>(&*CurrIter);
----------------
Any reason we don't just increment in the for statement itself?

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:774
@@ +773,3 @@
+    LoadInst *Load1 = dyn_cast<LoadInst>(MU->getMemoryInst());
+    // This isUsedOutsideOfBlock is also conservative
+    if (!Load1 || Load1->isUsedOutsideOfBlock(Succ1))
----------------
Please add a period.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:858
@@ +857,3 @@
+
+  // Skip all this if we don't have any memory accesses to look at
+  auto *Pred0Accesses = MSSA->getBlockAccesses(Pred0);
----------------
Please add a period.


http://reviews.llvm.org/D8688