[PATCH] D8688: Update MergedLoadStoreMotion to use MemorySSA

[Gerolf Hoflehner via llvm-commits]

Gerolf added a comment.

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.
I also take a (late :-() look at the core MSSA design. Could you outline the high-level MSSA design in more detail than http://reviews.llvm.org/D7864?Eg. how does it compare/relate to http://www.airs.com/dnovillo/Papers/mem-ssa.pdf?  Thanks!

-Gerolf


================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:346
@@ -271,1 +345,3 @@
   HoistedInst->insertBefore(HoistPt);
+  if (MSSA) {
+    // We also hoist operands of loads using this function, so check to see if
----------------
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. 

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:1032
@@ +1031,3 @@
+
+void MergedLoadStoreMotion::updateMemorySSAForSunkStores(
+    BasicBlock *SinkLocation, StoreInst *S0, StoreInst *S1, StoreInst *SNew) {
----------------

What got me here is that I expected the update to be simple. Why is here more involved than replacing two MD's with one, possibly removing a MP? From the clients perspective updates like this look too complicated to get right. 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. 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.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:1099
@@ +1098,3 @@
+
+  BasicBlock *MD0Block = MD0->getBlock();
+  for (auto UI = MD0->use_begin(), UE = MD0->use_end(); UI != UE;) {
----------------
There could be a lambda or function that takes care of the two loops.


http://reviews.llvm.org/D8688