[PATCH] D8688: Update MergedLoadStoreMotion to use MemorySSA

[Daniel Berlin via llvm-commits]

dberlin 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);
----------------
Gerolf wrote:
> The code from here could be a function. I prefer a function to fit on my screen.
Fixed

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:832
@@ +831,3 @@
+    bool SafeToLoadUnconditionally =
+        (LookupResult.first != LookupResult.second) &&
+        isSafeToLoadUnconditionally(Load1->getPointerOperand(),
----------------
Gerolf wrote:
> Why do you need this? When the first condition is false the while loop does not execute. But then SafeToLoadUnconditionally is not needed anyway
> 
It's moved here because it's loop-invariant.  That's also why the check matches the loop check, to avoid calculating it if the loop won't execute :)
Otherwise, we would calculate it on every loop iteration, when it doesn't change.



================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:848
@@ +847,3 @@
+        if (BB0HoistBarrier != 0 &&
+            DFSNumberMap.lookup(Load0) > BB0HoistBarrier)
+          continue;
----------------
Gerolf wrote:
> 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.
You are thinking about it backwards i think.
The dfs numbers are indeed as you  list them

If the block looks like this:

0
1 - load barrier
2 - load

We can't hoist. 

If it's 
0
1 - load
2 - load barrier

We can hoist.

So if DFS(load) > DFS(load barrier), that means the load barrier is *above* us in the block (and we come *after* the load barrier), and we can't hoist past it.




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

================
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)
----------------
Gerolf wrote:
> 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.
1. Yes, i'll fix.
2. This code was copied from mergeLoads originally. It is indeed too restrictive (there are a lot of random restrictions we could relax). I'll add a FIXME.


http://reviews.llvm.org/D8688