[PATCH] D21777: [MemorySSA] Switch to a different walker

[George Burgess IV via llvm-commits]

george.burgess.iv marked 9 inline comments as done.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:198
@@ +197,3 @@
+def_chain(MemoryAccess *MA, MemoryAccess *UpTo = nullptr) {
+  assert((!UpTo || find(def_chain(MA), UpTo) != def_chain_iterator()) &&
+         "UpTo isn't in the def chain!");
----------------
dberlin wrote:
> This seems like a really expensive assert, no?
> 
> (maybe it should be in XDEBUG or whatever expensive checking is these days)
Good point

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:229
@@ +228,3 @@
+        if (auto *MD = dyn_cast<MemoryDef>(MA)) {
+          HadDefinitiveClobber =
+              HadDefinitiveClobber || MSSA.isLiveOnEntryDef(MD) ||
----------------
dberlin wrote:
> Why not |= instead of HadDefinitiveClobber = HadDefinitiveClobber || :)
> 
I think `|=` wouldn't let us short-circuit, and given that `instructionClobbersQuery` isn't completely trivial, I think short-circuiting may be beneficial here. :)

If I'm wrong, I'm happy to swap to `|=` in a followup commit.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:357
@@ +356,3 @@
+
+      auto *Accesses = MSSA.getBlockAccesses(Node->getBlock());
+      if (Accesses) {
----------------
dberlin wrote:
> I assume this is not expensive in practice, because otherwise, you can just track it when we build memoryssa, and always have an answer to "lastnonuse for a block".
> 
> (even under updates, it's trivial to update in O(1))
> 
> Right now this looks N^2 since the dom tree above us could contain every block, and it could be all loads and then one store  at the top.
> 
> 
> otherwise, you can just track it when we build memoryssa, and always have an answer to "lastnonuse for a block".

Given that I ended up un-killing `findDominatingDef`, (and that this basically does the same job as `findDominatingDef` AFAICT), it may be a good idea to swap to that in the future, yeah. Will add a FIXME. :)

> Right now this looks N^2 since the dom tree above us could contain every block, and it could be all loads and then one store at the top.

FWIW, Because we have caching (`PhiTargetCache`), it's linear-time worst-case when we're initially optimizing uses. We kill that cache when updates happen, though, because it relies on the domtree/set of MemoryDefs not changing, so it may be n^2 there.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1626
@@ -983,2 +1625,3 @@
                                         DominatorTree *D)
-    : MemorySSAWalker(M), AA(A), DT(D) {}
+    : MemorySSAWalker(M), Walker(*M, *A, *D, Cache) /* heh */,
+      AutoResetWalker(true) {}
----------------
dberlin wrote:
> remove the heh :)
Oops :)


https://reviews.llvm.org/D21777