[PATCH] D55957: [MemorySSA] Refactor CachingWalker.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

This LGTM at a high level; just a handful of nits for you, and I think this should be good to submit



================
Comment at: lib/Analysis/MemorySSA.cpp:961
+                                              const MemoryLocation &);
+  MemoryAccess *getClobberingMemoryAccessBase(MemoryAccess *, bool);
+  void verify(const MemorySSA *MSSA);
----------------
nit: if we don't have commentary about SkipSelf elsewhere, can we please have a description of why it exists/what it's intended to do/etc.?


================
Comment at: lib/Analysis/MemorySSA.cpp:965
+protected:
+  friend class MemorySSA;
+  MemorySSA *MSSA;
----------------
Since this is already a nested class, is this necessary?

I don't use `friend` often enough to know for sure 


================
Comment at: lib/Analysis/MemorySSA.cpp:2225
+      return StartingAccess->getOptimized();
+    else
+      IsOptimized = true;
----------------
nit: no else after return, please


================
Comment at: lib/Analysis/MemorySSA.cpp:2281
+
+  LLVM_DEBUG(dbgs() << "Result Memory SSA clobber for " << *I << " is ");
   LLVM_DEBUG(dbgs() << *Result << "\n");
----------------
Please also note whether `SkipSelf` was true/false in these debug printouts


================
Comment at: lib/Analysis/MemorySSA.cpp:2287
+void MemorySSA::ClobberWalkerBase::verify(const MemorySSA *MSSA) {
+  Walker.verify(MSSA);
+}
----------------
nit: if member functions are one or two lines like this, please define them in the class itself (unless there's some code ordering-related reason not to)


================
Comment at: lib/Analysis/MemorySSA.cpp:2293
+  return Walker->getClobberingMemoryAccessBase(MA, false);
+}
+MemoryAccess *
----------------
please clang-format (I think it prefers empty lines between functions)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55957/new/

https://reviews.llvm.org/D55957