[PATCH] D8688: Update MergedLoadStoreMotion to use MemorySSA

[Daniel Berlin via llvm-commits]

On Wed, Jun 22, 2016 at 7:07 PM, Gerolf Hoflehner <ghoflehner at apple.com>
wrote:

> Gerolf added a subscriber: Gerolf.
>
> ================
> Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:297
> @@ -231,3 +296,3 @@
>
>      MemoryLocation Loc0 = MemoryLocation::get(Load0);
>      MemoryLocation Loc1 = MemoryLocation::get(Load1);
> ----------------
> Loc0,1 should be moved below the conditional. Then they are definitely
> needed.
>
> ================
> 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
> ----------------
> This is a MSSA update problem. How about
> if (MSSA && MSSA->update(HoistCan, HoistedInst))
>     MSSA->removeMemoryAccess(ElseInst);
>
>
This is a "is this a scalar problem". The only thing we are checking is
whether it is memory or not.. As for the way to  update, see below :)



> Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:1033
> @@ +1032,3 @@
> +
> +void MergedLoadStoreMotion::updateMemorySSAForSunkStores(
> +    BasicBlock *SinkLocation, StoreInst *S0, StoreInst *S1, StoreInst
> *SNew) {
> ----------------
> Shouldn't updates be owned by MSSA rather than each client?
>
>
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160622/679a819f/attachment.html>