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

[Daniel Berlin via llvm-commits]

dberlin 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);
----------------
What do you mean here?
That either sounds like a bug in memssa, or something :)

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:770
@@ +769,3 @@
+            // NewMemAcc before removing OldMemAcc.
+            OldMemAcc->replaceAllUsesWith(NewMemAcc);
+            MSSA->removeMemoryAccess(OldMemAcc);
----------------
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).



https://reviews.llvm.org/D22966