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

[Daniel Berlin via llvm-commits]

dberlin accepted this revision.
dberlin added a comment.
This revision is now accepted and ready to land.

Fix up the comments and LGTM


================
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!");
----------------
This seems like a really expensive assert, no?

(maybe it should be in XDEBUG or whatever expensive checking is these days)

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:206
@@ +205,3 @@
+/// Because it uses no cache, etc., it can get expensive.
+static void LLVM_ATTRIBUTE_UNUSED
+checkClobberSanity(MemoryAccess *Start, MemoryAccess *ClobberAt,
----------------
Please document what the parameters are supposed to be ;)

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:212
@@ +211,3 @@
+  SmallVector<MemoryAccessPair, 8> Worklist;
+  Worklist.emplace_back(Start, StartLoc);
+
----------------
Nit: Move this right before the while loop start.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:220
@@ +219,3 @@
+  bool HadDefinitiveClobber = false;
+  while (!Worklist.empty()) {
+    MemoryAccessPair MAP = Worklist.pop_back_val();
----------------
A comment on what, precisely, this is doing, would be helpful.


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:222
@@ +221,3 @@
+    MemoryAccessPair MAP = Worklist.pop_back_val();
+    // No revisiting.
+    if (!VisitedPhis.insert(MAP).second)
----------------
Why? (don't answer, just document :P)

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:229
@@ +228,3 @@
+        if (auto *MD = dyn_cast<MemoryDef>(MA)) {
+          HadDefinitiveClobber =
+              HadDefinitiveClobber || MSSA.isLiveOnEntryDef(MD) ||
----------------
Why not |= instead of HadDefinitiveClobber = HadDefinitiveClobber || :)


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:351
@@ +350,3 @@
+    while ((Node = Node->getIDom())) {
+      auto At = PhiTargetCache.find(BB);
+      if (At != PhiTargetCache.end()) {
----------------
This is probably better named WalkTargetCache :)

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:357
@@ +356,3 @@
+
+      auto *Accesses = MSSA.getBlockAccesses(Node->getBlock());
+      if (Accesses) {
----------------
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.



================
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) {}
----------------
remove the heh :)


https://reviews.llvm.org/D21777