[PATCH] D8688: Update MergedLoadStoreMotion to use MemorySSA

[Gerolf Hoflehner via llvm-commits]

Gerolf added inline comments.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:809
@@ +808,3 @@
+  // accesses in the first.
+  for (auto AI = Succ1Accesses->begin(), AE = Succ1Accesses->end(); AI != AE;) {
+    const MemoryUse *MU = dyn_cast<MemoryUse>(&*AI);
----------------
The code from here could be a function. I prefer a function to fit on my screen.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:832
@@ +831,3 @@
+    bool SafeToLoadUnconditionally =
+        (LookupResult.first != LookupResult.second) &&
+        isSafeToLoadUnconditionally(Load1->getPointerOperand(),
----------------
Why do you need this? When the first condition is false the while loop does not execute. But then SafeToLoadUnconditionally is not needed anyway


================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:848
@@ +847,3 @@
+        if (BB0HoistBarrier != 0 &&
+            DFSNumberMap.lookup(Load0) > BB0HoistBarrier)
+          continue;
----------------
Within a block I thought the DFS numbers are
0
1
2
...
So I would expect loop up(load) < BBHoistBarrier. I'm probably wrong about the DFSs.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:850
@@ +849,3 @@
+          continue;
+        unsigned int BB1HoistBarrier =
+            FirstHoistBarrier.lookup(Load1->getParent());
----------------
Load1 is loop invariant and the continue condition can be computed in the header.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:862
@@ +861,3 @@
+      MergedLoads |= Res;
+      // Don't attempt to hoist above loads that had not been hoisted.
+      if (Res)
----------------
That comment does not look right. Load0 and load1 just got hoisted and LookupIter->first could still be Load0.

Also, not hoisting above loads that had not been hoisted is too restrictive. With better/cheaper DA this optimization can be more aggressive. Perhaps this could be a FIXME.


http://reviews.llvm.org/D8688