[PATCH] D8688: Update MergedLoadStoreMotion to use MemorySSA

[Gerolf Hoflehner via llvm-commits]

Gerolf added a subscriber: Gerolf.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:297
@@ -231,3 +296,3 @@
 
     MemoryLocation Loc0 = MemoryLocation::get(Load0);
     MemoryLocation Loc1 = MemoryLocation::get(Load1);
----------------
Loc0,1 should be moved below the conditional. Then they are definitely needed.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:346
@@ -271,1 +345,3 @@
   HoistedInst->insertBefore(HoistPt);
+  if (UseMemorySSA) {
+    // We also hoist operands of loads using this function, so check to see if
----------------
This is a MSSA update problem. How about 
if (MSSA && MSSA->update(HoistCan, HoistedInst))
    MSSA->removeMemoryAccess(ElseInst);

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:542
@@ -448,1 +541,3 @@
     assert(S1->getParent() == A1->getParent());
+    if (UseMemorySSA)
+      updateMemorySSAForSunkStores(BB, S0, S1, SNew);
----------------
You could remove the conditional here and simply return from updateMemorySSA when MSSA is null.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:617
@@ -521,1 +616,3 @@
 
+// \brief DFS Number instructions in blocks in dominator order, tracking where
+// the first and last hoist barriers are.
----------------
How about: Visit blocks in DFS order and number instructions. For each block track first hoist barrier and last sink barrier.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:745
@@ +744,3 @@
+  DominatorTree *DT = &AM.getResult<DominatorTreeAnalysis>(F);
+  if (UseMemorySSA)
+    MSSA = &AM.getResult<MemorySSAAnalysis>(F);
----------------
I think this is the only place where you need UseMemorySSA. All other instances can be handled by if (MSSA).

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:1033
@@ +1032,3 @@
+
+void MergedLoadStoreMotion::updateMemorySSAForSunkStores(
+    BasicBlock *SinkLocation, StoreInst *S0, StoreInst *S1, StoreInst *SNew) {
----------------
Shouldn't updates be owned by MSSA rather than each client?


http://reviews.llvm.org/D8688