[PATCH] D8688: Update MergedLoadStoreMotion to use MemorySSA

[Daniel Berlin via llvm-commits]

dberlin marked 10 inline comments as done.

================
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"));
----------------
mcrosier wrote:
> 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.
I'll rename it to use-memoryssa-mldst (or whatever the shortname for this pass is).

A single flag would be nice, but would make it much harder to turn memoryssa on and off for each pass, and the point of the flag is to be able to test/debug regressions until we are satisified and get rid of the non-memoryssa version.

(ala what happened with SROA, etc and the ssaupdater)

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:183
@@ -116,1 +182,3 @@
     AU.setPreservesCFG();
+    AU.addRequired<TargetLibraryInfoWrapperPass>();
+    AU.addRequired<DominatorTreeWrapperPass>();
----------------
It is not, this is a merge error.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:189
@@ -119,2 +188,3 @@
+    AU.addPreserved<MemorySSAWrapperPass>();
     AU.addPreserved<MemoryDependenceWrapperPass>();
   }
----------------
This is not necessary, setPreservesCFG will preserve it, and all other passes marked CFG-only

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:762
@@ +761,3 @@
+    ++NLoads;
+    if (NLoads * Size1 >= MagicCompileTimeControl)
+      break;
----------------
mcrosier wrote:
> Pre-increment here?
Not sure what line this is supposed to go with, i can't find a post-increment around :)

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:769
@@ +768,3 @@
+    auto CurrIter = AI;
+    ++AI;
+    const MemoryUse *MU = dyn_cast<MemoryUse>(&*CurrIter);
----------------
mcrosier wrote:
> Any reason we don't just increment in the for statement itself?
Fixed. Originally, the updater was modifying the accesslist we were walking on and invalidating the iterator.
It turns out to be non-trivial to change this, but i never moved this loop back to account for this.


http://reviews.llvm.org/D8688