<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jun 22, 2016 at 7:07 PM, Gerolf Hoflehner <span dir="ltr"><<a href="mailto:ghoflehner@apple.com" target="_blank">ghoflehner@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Gerolf added a subscriber: Gerolf.<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:297<br>
@@ -231,3 +296,3 @@<br>
<br>
     MemoryLocation Loc0 = MemoryLocation::get(Load0);<br>
     MemoryLocation Loc1 = MemoryLocation::get(Load1);<br>
----------------<br>
Loc0,1 should be moved below the conditional. Then they are definitely needed.<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:346<br>
@@ -271,1 +345,3 @@<br>
   HoistedInst->insertBefore(HoistPt);<br>
+  if (UseMemorySSA) {<br>
+    // We also hoist operands of loads using this function, so check to see if<br>
----------------<br>
This is a MSSA update problem. How about<br>
if (MSSA && MSSA->update(HoistCan, HoistedInst))<br>
    MSSA->removeMemoryAccess(ElseInst);<br>
<br></blockquote><div><br></div><div>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 :)<br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><br>
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:1033<br>
@@ +1032,3 @@<br>
+<br>
+void MergedLoadStoreMotion::updateMemorySSAForSunkStores(<br>
+    BasicBlock *SinkLocation, StoreInst *S0, StoreInst *S1, StoreInst *SNew) {<br>
----------------<br>
Shouldn't updates be owned by MSSA rather than each client?<br>
<br></blockquote><div><br></div><div>That would be inconsistent with what we do in the rest of LLVM :)<br></div><div>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.</div><div>You can see that splitCriticalEdge takes a memdep argument, for example, and will update memdep instead of memdep updating itself.<br></div><div>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!</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div>In retrospect, i agree.<br></div><div>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.</div><div><br></div><div>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.</div><div><br></div></div></div></div>