<div dir="ltr">Also, ust to give you some idea of general update complexity:  If you don't bound the problem with some common cases, it's simply not possible to make it faster than current creation (Ie it would be simpler/easier to not preserve memoryssa in those cases).<div><br></div><div><br><div><br></div><div>For the common cases, assuming you tell us what variables to remove and what variables were added in what blocks:</div><div>1. If you only removed and replaced variables (Ie literally no code motion or insertion), it requires the renaming pass only.  It could be further optimized to not redo use chain optimization for memoryuses if you can guarantee you only touched memoryuses. For memorydefs, if you can guarantee (and you probably can't) that none of the aa relationships have changed, you would not have to redo use chain opt there either.  You can't guarantee this because basicaa, et al have walkers with random hard limits in them, and so you may get better results simply by reducing the length of a chain of things it had to walk, even if there was in fact, no aliasing either way.</div><div><br></div><div><div>2. if the cfg has not changed, and you have added/removed variables: if you added memoryuses, if we stay with pruned ssa, we have to recompute the IDF. We could minimize the size we calculate by tracking some things related to where the last uses in the program are relative to the iterated dominance frontier. It's complicated however.</div></div><div>After recomputing IDF, we also have to redo renaming. For inserting  memoryuses, if we tracked the first/last version of a block (and updated it through the creation/removal apis), we only have to rename parts of the dom tree where the first/last version changes.</div><div><br></div><div>(IE if we changed first version in a block due to phi node insertion, but last version is the same, we do not have to rename children of that block.  if we changed last version but not first version, we only have to rename children but not the current block)</div><div>For inserting stores, it's something close to this but a bit more complicated.</div><div><br></div><div>3. if the cfg has changed, without tracking a lot more under the covers related to merge/join sets, it's not possible to make it faster than doing it from scratch.<br></div><div><br></div><div>The TL;DR of all this is that a general update mechanism is very hard to make faster than for scratch without giving it all kinds of specific info, and usually figuring out that info is as hard as figuring out the update algorithms, in my experience.</div></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 24, 2016 at 10:41 PM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Jun 24, 2016, 5:16 PM Gerolf Hoflehner <<a href="mailto:ghoflehner@apple.com" target="_blank">ghoflehner@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Gerolf added a comment.<br>
<br>
I'll take a more in depth look into the load and store merge routines also. At a first glance it seems  one could just add a few MSSA hooks rather than copy-paste-modify the code base.<br></blockquote></div></span><div>It is possible, but th<span style="font-size:13px">e other version is o(n^3), the memory SSA version is o(n), so this is deliberate. It is not possible to make the other version less than n^2 without memory versions.</span></div><span class=""><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px"><br></span></div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I also take a (late :-() look at the core MSSA design. Could you outline the high-level MSSA design in more detail than <a href="http://reviews.llvm.org/D7864?Eg" rel="noreferrer" target="_blank">http://reviews.llvm.org/D7864?Eg</a>. </blockquote></div><div><br></div></span><div>There are also a bunch of mailing list threads and the comment in memoryssa.h, but happy to expand on whatever for you :)</div><span class=""><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">>how does it compare/relate to </span><a href="http://www.airs.com/dnovillo/Papers/mem-ssa.pdf" rel="noreferrer" style="font-size:13px" target="_blank">http://www.airs.com/dnovillo/Papers/mem-ssa.pdf</a><span style="font-size:13px">?  Thanks!</span></div><div><span style="font-size:13px"><br></span></div></span><div><span style="font-size:13px">Sure.</span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">This paper covers what was the first version of memoryssa for GCC.  Over time, we significantly simplified the design after getting years of experience with clients.   GCC's current design is now identical to the llvm one.</span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">Compared to the paper, in both implementations, there are no kills anymore, and no partitioning. All loads and stores use and define a single memoryssa variable instead of multiple ones representing heap partitions.   There is a long discussion of why in one of the threads, but it boils down to: it's not worth making it more precise for the cost you pay in time or memory or reasoning or complexity of code, for what clients really ended up wanting out of it.  </span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">This pass is a good example. Having multiple variables  would not make it faster or more precise for load or store  merging  and may in fact make it much slower.  For passes that care, the walker interface provides a way to further try to disambiguate def def and def use chains.</span></div><span class=""><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px"><br></span></div><div><br></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px"><br></span></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
-Gerolf<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:346<br>
@@ -271,1 +345,3 @@<br>
   HoistedInst->insertBefore(HoistPt);<br>
+  if (MSSA) {<br>
+    // We also hoist operands of loads using this function, so check to see if<br>
----------------<br>
Ok, it would probably not be possible foresee all use cases. I certainly agree to the iterative approach you propose. I'm surprised though that a simple replacement of a load has not come up elsewhere.<br></blockquote></div><div><br></div></span><div>This is the first major pass conversion outside of earlycse. I am working pass by pass to preserve it for gvn.</div><div><br></div><div>Note that this is also not simple load replacement, it's merging multiple loads into a single one by insertion of a new load and removing.  However, the operations performed are exactly what is happening to the non memoryssa ir, which is that you are inserting a new load, and removing the two old ones.</div><span class=""><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:1032<br>
@@ +1031,3 @@<br>
+<br>
+void MergedLoadStoreMotion::updateMemorySSAForSunkStores(<br>
+    BasicBlock *SinkLocation, StoreInst *S0, StoreInst *S1, StoreInst *SNew) {<br>
----------------<br>
<br>
What got me here is that I expected the update to be simple.</blockquote></div><div><br></div></span><div>It would be if the stores were not may defs. Because they are, there may be more of them below in the same block that you are not touching.</div><div><br></div><div>Ie.  </div><div>1=memorydef(0)</div><div>Store a </div><div>2= memorydef(1)</div><div>Store b</div><div><br></div><div>When you move a, you must update b's defining access to 0</div><div><br></div><div>It is not possible to avoid this in all cases.   Even if you had multiple memoryssa variables, there is no perfect partitioning of the heap that guarantees you won't have a use.</div><div><br></div><div>If you removed the defining access of may defs, you would not easily know how far a given memorydef version is valid for, or be able to tell the may defs that might kill your store.  In the above design, finding the lowest sink points  and uses  for a given store is easy, because you can walk the use chains.    This makes a number of optimization algorithms that are otherwise quadratic, linear.</div><span class=""><div><br></div><div><br></div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Why is here more involved than replacing two MD's with one, possibly removing a MP? </blockquote></div><div><br></div></span><div>In the simple case, where there are no stores below you, it is in fact, that :)</div><div><br></div><div>The other complication is that a mp may already exist in the block below you (imagine our store a, store b case in both sides of the diamond. A phi will exist in the merge block and your insertion point is after the phi)</div><span class=""><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From the clients perspective updates like this look too complicated to get right. </blockquote></div></span><div>Fwiw: I disagree, but I admit I've written the memoryssa update routines for a ton of  both gcc and llvm passes.  This is not materially different from the ir update you have to perform if you sink stores and their operands with pre.</div><div>This is pretty much as complicated as it gets as well, because it's code motion, not just replacement with existing values.</div><span class=""><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Maybe there is verifier in place, but in the code I don't even see a single assertion but could help debugging issues should something go wrong.</blockquote></div></span><div>There is a verifier that is run in both the updating  unit tests and the test cases for this pass that verify the updating is correct.</div><div>Past that, removal and insertion routines will assert if you do something that is obviously wrong, much more than the ir does The verifier in both cases is used to catch screwups that cost too much to verify at all times.</div><div>The verifier can be called at any time, and I'm happy to add a call to it here under xdebug if you like.</div><span class=""><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Perhaps we can come up with some utility routines that make simpler to grasp: when two memory or more memory operation get moved, this is the blueprint for updating MSSA and guarantee it stays consistent.<br></blockquote></div><div><br></div></span><div>I think we could in general, but probably not for this kind of specialized code motion.</div><div><br></div><div>The simple cse case is already pretty simple (for loads, you do nothing but remove the dead load. For stores , replacealluseswith your definingacess, Remove the dead store).</div><div><br></div><div>Note that only stores are ever complex to reason about when updating. Because loads are just uses, they never require much because doing things do them can't change the reaching versions or require new phi nodes (unless you sink loads instead of hoist them, because both memoryssa and the normal ir are pruned ssa, so they eliminate phi nodes that have no uses. We could just turn this back off if we wanted to guarantee no load sinking would ever require a new phi node, but I have not hit a pass that wants to sink loads)</div><div><br></div><div>For stores, general downward code motion can get complex enough that it may be simpler and easier to not try to preserve the analysis.   This is true of normal ssa as well.   This pass is pretty much the limit of what I'd try to preserve rather than have it just recalculate memoryssa. The set of update cases is fixed and formally provable.</div><div><br></div><div>But thoughts definitely welcome.</div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
================<br>
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:1099<br>
@@ +1098,3 @@<br>
+<br>
+  BasicBlock *MD0Block = MD0->getBlock();<br>
+  for (auto UI = MD0->use_begin(), UE = MD0->use_end(); UI != UE;) {<br>
----------------<br></span>
There could be a lambda or function that takes care of the two loops..<br></blockquote></div><div><br></div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<a href="http://reviews.llvm.org/D8688" rel="noreferrer" target="_blank">http://reviews.llvm.org/D8688</a><br>
<br>
<br>
<br>
</blockquote></div></blockquote></div><br></div>