[PATCH] D19503: [MemorySSA] Overhaul CachingMemorySSAWalker's cache to allow more selective cache invalidation

[Daniel Berlin via llvm-commits]

dberlin added a comment.

In http://reviews.llvm.org/D19503#423119, @reames wrote:

> Just to make sure I'm understand the context correctly, we're removing a memorydef which has been determined to be dead (i.e. dse) and as a result, leaving the memory phi in place pointing at the definition before the one we removed is correct but not necessarily optimally precise?  That seems reasonable.  Possibly part of the API should be the precision required?


This is actually a violation of the documentation we have, which states that memoryuses will point to the thing that *actually* clobbers them.

If we can't make this work in the new caching scheme, we should fix the new caching scheme :)

However, i don't think this is that hardto make work.

Figuring out the set of invalidations is easy, even if the number may be large.

These are all Value's now, so they have Users and Uses.

For a memoryuse invalidation, you do nothing.
for a memorydef invalidation, invalidate the cache for all uses, recursively (IE recurse on memorydefs).

That is the set of things it may have changed.

This seems like it would be bad, but it won't be that bad, since most people don't ask about stores (memorydefs), and loads already point to their clobbering definitions (and thus, fwiw, caching them outside of the memoryssa builder is pointless)

> Also, the presence of both code motion and semantic changes makes this essentially impossible to review quickly.  Please make the code motion changes separately, land them, and then update the patch.  If we disagree about where the code lives, we can move it again later.  Your suggestion seems reasonable enough and I doubt we'll care too much.  :)


+1 to this.


http://reviews.llvm.org/D19503