[PATCH] D8688: Update MergedLoadStoreMotion to use MemorySSA

[Daniel Berlin via llvm-commits]

dberlin added inline comments.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:1033
@@ +1032,3 @@
+
+void MergedLoadStoreMotion::updateMemorySSAForSunkStores(
+    BasicBlock *SinkLocation, StoreInst *S0, StoreInst *S1, StoreInst *SNew) {
----------------
dberlin wrote:
> 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.
(and, just to also point out, the update algorithm we use is specific to diamonds, and would not work for more general control flow. It also would not work without the set of legality testing, etc, we perform elsewhere in the code )


http://reviews.llvm.org/D8688