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

[George Burgess IV via llvm-commits]

george.burgess.iv created this revision.
george.burgess.iv added reviewers: dberlin, hfinkel, reames.
george.burgess.iv added a subscriber: llvm-commits.

This patch introduces a new cache for MemorySSA that allows us to invalidate cache entries directly related to a given MemoryAccess in constant time.

We do this by making cache a directed graph. Each node is a MemoryAccess, and each edge is a cache entry. In the case of calls, each node can only have one edge to another node. In the case of Values, nodes may have multiple edges, each tagged by a MemoryLocation. 

The only case in which we need to invalidate anything (at the moment) is when a MemoryAccess has been removed from the graph. We handle this by flipping an invalid bit in the corresponding node, and keeping the node alive. Edges to invalid nodes are removed as they are discovered during cache lookups. This means we don't basically have to roll our own Use-lists for this cache.

The moving of CachingMemorySSAWalker was just because I didn't feel that this cache needed to be in MemorySSA.h. If we feel that the caching walker's definition should remain in MemorySSA.h, I'm happy to refactor things so that's possible.

Running this across clang+LLVM with MemorySSA's verifier enabled yields 0 assertion failures.

I had to disable a part of a test for this to work. This is because, given the following code:

```
define void foo(i32* %i) {
entry:
  br i1 undef, label %if.then, label %if.end
if.then:
  ; MemoryDef
  store i32 0, i32* %i
  br label %if.end
if.end:
  ; MemoryPhi
  ; MemoryUse(Phi)
  load i32 %i
  ret void
}
```

...When the MemoryDef is removed, the test asserts that we should be able to invalidate the MemoryUse's cache entry (which points to the Phi) because the Def was removed. I think this is sane, but in order for it to work well, it currently requires basically invalidating the cache entries of all MemoryAccess reachable from the clobber we're deleting, unless we implement either:
- Automatic Phi deletion when all of a Phi's upward defs point to the same place (wouldn't be a full fix, but would probably be a simple one), or
- More information sharing between the walker and cache, so we know what each cache entry is *actually* dependent on.

I think this is necessary because, given the following CFG:

```
     A
   /   \
  B     |
 / \    |
C   D   E
 \ /    |
  E    /
    \ /
     F
```


...If we're trying to optimize a use in F that would be liveOnEntry if not for a clobber in C, there's really no way I can see to signal that the removal of the clobber in C should invalidate the use in F without basically invalidating every MemoryAccess that is reachable from C. Presumably, most of the things invalidated by that wouldn't care about that particular clobber, so we'd likely end up doing a ton of recomputation for not much (if any) gain.

We may be able to solve this problem by making the walker explicitly note that the Def in C caused optimization of the use in F to fail, but that's probably a much more involved change to the walker. Given that "overhaul the walker" is item #3 on my MemorySSA-related to-do list, I think that can be done later if we believe it would be valuable.

http://reviews.llvm.org/D19503

Files:
  include/llvm/Transforms/Utils/MemorySSA.h
  lib/Transforms/Utils/MemorySSA.cpp
  unittests/Transforms/Utils/MemorySSA.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D19503.54914.patch
Type: text/x-patch
Size: 21879 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160425/6c45c139/attachment.bin>