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

[Sebastian Pop via llvm-commits]

sebpop added inline comments.

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:761
@@ +760,3 @@
+            // otherwise when we try to remove OldMemAcc, MSSA gets confused and
+            // removes NewMemAcc as well.
+            MSSA->removeMemoryAccess(OldMemAcc);
----------------
dberlin wrote:
> What do you mean here?
> That either sounds like a bug in memssa, or something :)
My first patch had both MemUses and MemDefs handled in the same way.
I had a hard time finding how to get around this, so I would rather amend the MSSA to avoid other people getting through the same problems.
Let me think how to fix this in MSSA.

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:770
@@ +769,3 @@
+            // NewMemAcc before removing OldMemAcc.
+            OldMemAcc->replaceAllUsesWith(NewMemAcc);
+            MSSA->removeMemoryAccess(OldMemAcc);
----------------
dberlin wrote:
> Are you guaranteed that anything that conflicted with the old store conflicts with this store by your hoist safety rules?
> (if not, you should only replace  inMD's and phis, not memoryuses)
> 
> it might be worth either giving an example here, or adding a testcase.
> 
> The two problems are:
> 
> 1 = MemoryDef<liveOnEntry>
> store 
> 2 = MemoryDef<1>
> store
> 
> later
> memoryphi(2, )
> 
> 
> If the hoist point is 1, and you simply move the store, you will now have:
> 1 = MemoryDef<liveOnEntry>
> store 
> 3 = MemoryDef<1>
> store
> 2 = MemoryDef<1>
> store
> 
> later
> memoryphi(2, )
> 
> 
> This is wrong.  3 is now disconnected from the graph of reaching defs.
> 
> Additionally, if the hoist point is 2, you end up with:
> 1 = MemoryDef<liveOnEntry>
> store 
> 2 = MemoryDef<1>
> store
> 3 = MemoryDef<1>
> store
> 
> later
> memoryphi(2, )
> 
> The memoryphi is now wrong (and you disconnected 2 from the reaching graph).
> 
I added that comment above Def to make sure it is clear that hoisting guarantees these properties:

          // The definition of this ld/st will not change: ld/st hoisting is
          // legal when the ld/st is not moved past its current definition.
          MemoryAccess *Def = OldMemAcc->getDefiningAccess();

> hoist point is 1
Cannot happen: we cannot move the store "3" past store "2" as they are on the same chain (dependent.)
This is an illegal case of hoisting and will be discarded.

> hoist point is 2
Cannot happen either as there is a use of "2" (the phi node) past which "3" cannot be moved.
Again the dependence analysis will discard this hoisting as invalid.



https://reviews.llvm.org/D22966