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

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added inline comments.


================
Comment at: lib/Analysis/MemorySSA.cpp:1803
+  if (Walker)
+    Walker->verify(this);
   // Previously, the verification used to also verify that the clobberingAccess
----------------
chandlerc wrote:
> asbirlea wrote:
> > george.burgess.iv wrote:
> > > (I'd be equally happy with `getWalker()->verify(this)`, personally, since I agree that this is a tricky edge case :) )
> > `getWalker()` is not `const`, and I wanted to keep the `const` qualifier on the `verifyMemorySSA()`. Initializing the walker when building, so I removed the check.
> Would it be cleaner for the verify to build its *own* walker? It isn't in the fast path and that would insulate it from one or the other decision?
> 
> (There probably is a good reason to not do this, mostly asking to learn more about MemorySSA and the walker design.)
The reason is that `MemorySSA` has to be `move`able, and Walkers store a pointer to their `MemorySSA` instance. `Walker::verify` is "make sure our internal pointer to MemorySSA is the same thing as our parameter and hasn't been moved, because oh geez if it's pointing somewhere else..."

Constructing a new one just to do that check is sort of redundant.

In general, I don't see how one can use MSSA without the Walker very effectively, so I don't think it's a bad idea to just have the ctor build it Once And For All. We can do a cleaner job with it, but again, that's probably best suited for a follow-up CL, since this one's pretty large as-is.

Since you asked for history, though: :)

It used to be that the walker carried a huge cache with it, so keeping it around was necessary. IIRC, it's now kept around primarily for convenience and microoptimizations: it's a relatively heavy alloc (1KBish?) on its own, since it carries with it a few large `Small` containers. For functions large enough to make those `Small` containers go to the heap, it's likely those containers will need that extra space again, so dropping them ASAP seems counterproductive.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59315