[PATCH] D8688: Update MergedLoadStoreMotion to use MemorySSA

[Daniel Berlin via llvm-commits]

dberlin marked 10 inline comments as done.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:346
@@ -271,1 +345,3 @@
   HoistedInst->insertBefore(HoistPt);
+  if (UseMemorySSA) {
+    // We also hoist operands of loads using this function, so check to see if
----------------
Gerolf wrote:
> This is a MSSA update problem. How about 
> if (MSSA && MSSA->update(HoistCan, HoistedInst))
>     MSSA->removeMemoryAccess(ElseInst);
As mentioned, if you look early enough in the thread on this, you can see at one point it was, in fact that API, and i was explicitly asked to make simpler apis instead of these kinds of composed ones :)

The way it is now is also consistent with how we do updates for everything else both SSA related (where you do exactly what you see below), and analysis related, in LLVM.
(IE memdep has you call invalidateCachedPointerInfo  and such, dominators requires you set and redirect dominators on your own)

In fact, if memoryssa was part of the IR, there would be no difference between the way this code looks and the rest of the function (which is updating the IR) looks . Because it's a side datastructure the naming is a little different and we haven't built all the same utilities yet.  

Additonally, the kind of API you suggest above would have wildly varying complexity and is overkill vs building utilities that handle the common cases, which is what we do elsewhere for ssa/dom/etc updates. 

I'm more than happy to commit to building those utilities as we come across common cases.



================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:1033
@@ +1032,3 @@
+
+void MergedLoadStoreMotion::updateMemorySSAForSunkStores(
+    BasicBlock *SinkLocation, StoreInst *S0, StoreInst *S1, StoreInst *SNew) {
----------------
Gerolf wrote:
> Shouldn't updates be owned by MSSA rather than each client?
(replying here since phab is not good at processing email like this):


That would be inconsistent with what we do in the rest of LLVM :)
MemorySSA is an SSA form, and like SSA in LLVM, passes are responsible for keeping SSA up to date.  This is also true of most analysis preservation.
You can see that splitCriticalEdge takes a memdep argument, for example, and will update memdep instead of memdep updating itself.
Same with dominators. Passes that want to preserve Dom are responsible for redirecting the dominators where they should go. They can screw it up, too!

Doing general updates (IE "I have no idea what happened, you go find out and fix it") is also expensive, and at least for every pass i've converted so far, completely unnecessary.

I also had more functional updaters (IE replaceAccessWithHoistedAccess) in memoryssa at earlier points, but if you look earlier in the review, you'll see others wanted it to be simple functionality instead.
In retrospect, i agree.
We can build a general updater if we need it, and we can build utilities to handle common updates if we need it.  So far, it hasn't been necessary.

At some point, the better question is really "why is memoryssa a side data structure instead of just part of the normal IR". But that's a question for another day.


http://reviews.llvm.org/D8688