[PATCH] D22966: GVN-hoist: compute MSSA once per function (PR28670)

[Daniel Berlin via llvm-commits]

dberlin added inline comments.

================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:759
@@ +758,3 @@
+                                                   MemorySSA::End);
+          OldMemAcc->replaceAllUsesWith(NewMemAcc);
+          MSSA->removeMemoryAccess(OldMemAcc);
----------------
The comment is not correct for stores.
There are other requirements.
Imagine the following case:

A
 | \
 | B
 | /
C

C has load %p2
B has store %p2
A has store %p2.


You will have:


 

```
 A:
  1 = MemoryDef(liveOnEntry)
   store %p2
 B: 
   2 = MemoryDef(1)
   store %p2
 C:
   3= MemoryPhi(1, 2)
   MemoryUse(3)
   load %p2

``` 


If you hoist the store in B to A, your code will give:

```
A:
 1 = MemoryDef(liveOnEntry)
  store %p2
 2 = MemoryDef(1)
  store %p2
B: 
  
C:
  3= MemoryPhi(1, 2)
  MemoryUse(3)


```
This is not correct, it should be MemoryPhi(2, 2) at worst, and in MemorySSA, we require you kill the phi.

code to do that:
 
```
      MemoryAccess *SamePhiOp = nullptr;
      for (auto &Op : Phi->incoming_values()) {
        if (!SamePhiOp)
          SamePhiOp = cast<MemoryAccess>(Op.get());
        else if (SamePhiOp != Op.get()) {
          SamePhiOp = nullptr;
          break;
        }
      }
      // If this has some value, it means all operands of the phi are now the
      // same
      if (SamePhiOp) {
        Phi->replaceAllUsesWith(SamePhiOp);
        MSSA->removeMemoryAccess(Phi);
      }
```
Please make sure this happens, and use print-memoryssa to verify memoryssa is updated properly in testcases.



https://reviews.llvm.org/D22966