[PATCH] D21777: [MemorySSA] Switch to a different walker

[George Burgess IV via llvm-commits]

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

This patch switches MemorySSA to use a new walker. The motivating reasons for this are:

- Accuracy: the old walker cached things *really* eagerly, so it had to be overly conservative in many cases.
- Speed of phi optimization: in some cases, the old walker would walk *way* more than it had to in order to determine if a phi was optimizable or not. This walker takes a more incremental approach, so we do as little work as we reasonably can.
- Flexibility: this walker can be extended without much effort to do things like getting all clobbers that block a phi optimization. This will supposedly be useful in the future. Also, phi optimization is split into its own little world, so we can easily switch it off entirely if we want to (e.g. for -O1).
- Testability: we can turn the cache on/off, so it's trivial to check whether the cache causes us to give a different answer than manually walking.

The big parts of this patch (in terms of line count) are caching and phi optimization. Phi optimization is more or less an iteratively expanding DFS; given a phi `P`, we figure out what the nearest legal optimization of `P`, `Q`, would be, then we see if there are any clobbers on any path from `P` to `Q`. If so, we quit. If not, we walk from `Q` to its dominating phi, and repeat. Caching is done after we've done all walking. Caching is just difficult in general. :)

This patch also includes a stupidly simple verify-this-optimization-isn't-broken function.

This patch verifies with EXPENSIVE_CHECKS enabled for MSSA.cpp when bootstrapping all of clang/LLVM.

Other smaller (but still noteworthy) things:

- With this patch, we shouldn't cache MemoryUses anymore. This is desirable, since they always point to their clobber, so having them in the cache was kind of redundant
- When running this across LLVM/clang, I didn't notice a major speed difference between this and the old walker. A script tells me it's in the noise, but note that the test wasn't quite scientific (read: `time ninja -j$((NCORES * 1.1))`)
- The explicit call caching was removed from the upwards query. If it needs to be there, it can be readded without much effort, but the goal was to keep this patch as simple as I could, while not dropping accuracy.
- The `ClobberWalker::reset()` function exists because an idea that's been thrown around is a bulk-update MSSA API (or some kind of thing where we'll do N updates back-to-back). If we don't need to drop the walker's cache of BB -> nearest-optimizable-access, then it shouldn't be terrible to keep it

Finally, note that there were some refactors split out from this, so it's easier to review. If you want to run things locally, you'll need to apply this on top of D21776.

http://reviews.llvm.org/D21777

Files:
  include/llvm/Transforms/Utils/MemorySSA.h
  lib/Transforms/Utils/MemorySSA.cpp
  test/Transforms/Util/MemorySSA/cyclicphi.ll
  test/Transforms/Util/MemorySSA/phi-translation.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D21777.62045.patch
Type: text/x-patch
Size: 39358 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160628/87e8e2c7/attachment.bin>