[PATCH] Update MergedLoadStoreMotion to use MemorySSA

[Philip Reames]

FYI, I'm basically reviewing this as a new algorithm.  Trying to match it up with what's there before doesn't seem particularly worth doing given the algorithmic problems you commented on.


================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:106
@@ +105,3 @@
+
+struct StoreInstHash {
+  size_t operator()(const StoreInst *A) const {
----------------
Can you use lambdas for these?  I'm not entirely sure that works, but if it does, the code would be cleaner.

Also, I'm surprised that a generic instruction hash function doesn't already exist.  What does GVN use for this?

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:117
@@ +116,3 @@
+  bool operator()(const StoreInst *A, const StoreInst *B) const {
+    return (A == B || A->isSameOperationAs(B));
+  }
----------------
Is the extra A == B check needed?  I would expect that to be inside isSameOperationAs.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:308
@@ -311,1 +307,3 @@
 
+  // We also hoist operands, so see if this is even a memory access we want to
+  // fix up
----------------
Not quite clear what you're trying to say with this comment?  The code appears to just be updating MemorySSA for the transform performed.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:410
@@ +409,3 @@
+    // We know that if the load has the same clobbering access as this one,,
+    // they must not be killed until the same point
+
----------------
I'm a bit confused here.  If we found the set of load instructions which share the generation number at the start of a block and we're merging two blocks with a single shared successor, why do we need any further legality checks?  Aren't all the loads guaranteed to have the generation number at the end of the common block?

Are you possibly trying to do two generations at once here?  Or something else more complex?

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:516
@@ +515,3 @@
+    MemoryAccess *Phi = MSSA->getMemoryAccess(BB);
+    MemorySSA::AccessListType::iterator Place(Phi);
+    ++Place;
----------------
Are you guaranteed there's not another phi here?  Not entirely sure what this is doing.  It may be completely correct, but it looks suspicious when compared to instruction insertion into a basic block.  

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:558
@@ -540,1 +557,3 @@
 
+  StoreInfo.reserve(Pred0->size());
+
----------------
Given this is a linear traversal of Pred0, does the reserve really gain you anything here?

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:568
@@ +567,3 @@
+  auto *TAccesses = MSSA->getBlockAccesses(T);
+  if (!TAccesses || !isa<MemoryPhi>(&TAccesses->front()))
+    return false;
----------------
Neat!

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:579
@@ -550,3 +578,3 @@
 
-    // Sink move non-simple (atomic, volatile) stores
-    if (!isa<StoreInst>(I))
+  for (auto AI = Pred0Accesses->rbegin(), AE = Pred0Accesses->rend(); AI != AE;
+       ++AI) {
----------------
Ok, I'm again a bit confused by the algorithm.  :)

What I expected to see was something along the following:
- Find store in each block which is a candidate for merge (using memory-def edges from MemoryPhi)
- Merge any loads in those basic blocks which use the generation if possible.
- Search downward from each store to find a conflicting memory access or end of block.  (This could either by by walking instructions or mem-use edges.)
- If both reach end of block, merge.
- Repeat previous steps with newly created MemoryPhi for earlier stores if any.

http://reviews.llvm.org/D8688

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/