[PATCH] D59315: [AliasAnalysis] Second prototype to cache BasicAA / anyAA state.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Overall approach LGTM for this selective caching behavior.

CFLAA bits LGTM. Just a few nits about MSSA



================
Comment at: lib/Analysis/MemorySSA.cpp:1165
       SkipWalker(nullptr), NextID(0) {
-  buildMemorySSA();
+  this->AA = nullptr;
+  BatchAAResults BatchAA(*AA);
----------------
nit: can this be simplified to `AA(nullptr)` above, then setting `this->AA = AA;` after we're done with `buildMemorySSA`?


================
Comment at: lib/Analysis/MemorySSA.cpp:1169
+  this->AA = AA;
+  // BatchAA dies here.
 }
----------------
nit: this comment seems redundant to me. If you feel like it's useful, please go into a bit more detail about how we only intend to use BatchAA while building, etc.


================
Comment at: lib/Analysis/MemorySSA.cpp:1480
 
-  CachingWalker *Walker = getWalkerImpl();
+  auto WalkerBase =
+      llvm::make_unique<ClobberWalkerBase<BatchAAResults>>(this, &BAA, DT);
----------------
nit: can these both just be stack-allocated?


================
Comment at: lib/Analysis/MemorySSA.cpp:1487
 
-  OptimizeUses(this, Walker, AA, DT).optimizeUses();
+  // Note: Optionally create the walker for future users. Then we can remove the
+  // "if(Walker)" in verifyMemorySSA.
----------------
If you want to do this, please do it as a part of this CL (looks like it'll require plumbing `AA` through). Otherwise, please remove the note


================
Comment at: lib/Analysis/MemorySSA.cpp:1803
+  if (Walker)
+    Walker->verify(this);
   // Previously, the verification used to also verify that the clobberingAccess
----------------
(I'd be equally happy with `getWalker()->verify(this)`, personally, since I agree that this is a tricky edge case :) )


================
Comment at: lib/Analysis/MemorySSA.cpp:2337
+MemorySSA::CachingWalker<AliasAnalysisType>::getClobberingMemoryAccess(
+    MemoryAccess *MA) {
   return Walker->getClobberingMemoryAccessBase(MA, false);
----------------
nit: this is becoming a lot of ceremony for 1-line methods. I know there are subtleties here about declaration order that require us to define some things out-of-line, but if that isn't the case, can you please inline these methods into their class definitions?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59315