Page MenuHomePhabricator

This patch introduces MemorySSA, a virtual SSA form for memory. Details on what it looks like are in MemorySSA.h
ClosedPublic

Authored by george.burgess.iv on Feb 24 2015, 12:11 PM.

Details

Summary

There is no grand scheme to use this across LLVM (at least, that i have).
The main purposees for it are to clean and speed up things like MemoryDependenceAnalysis
(which spends a lot of time walking to find use/defs of memory, etc), and to
be able to provide load value numbering to GVN (and PRE).
Secondarily, it may be possible to speed up some of the other load/store
motion passes using it.

Note that creation of MemorySSA is linear time and space relative to the IR
size. It uses the same linear time phi placement algorithm PromoteMemoryToRegisters does
(though sadly, not the same code because abstracting PromoteMemoryToRegisters
is not easy since we don't derive the memory access classes from things
like Value, etc)

It's lived both as a utility that passes could create and delete, and as a pass.
In the current patch, it's a functionpass that things can require
as an analysis.
Happy to put it back to being a utility, not sure which makes more sense.
I only have GVN using it right now.

As a small note: The immediate-use lists are ugly right now. Suggestions for how to do
that better welcome.

Anyway, happy to answer any questions, and not looking to commit this tomorrow,
more it got to the point where i think it would be good to get some other eyes
on it.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
reames added inline comments.Apr 13 2015, 10:33 AM
include/llvm/Analysis/AliasAnalysis.h
148 ↗(On Diff #23347)

This can and should go in separately.

371 ↗(On Diff #23347)

Same as above.

Ok, here's the MemorySSA.cpp parts of the review. Lots of small style comments, a couple of larger concerns. At least some of the code duplication would be nice to address before submission, though I wouldn't say it's mandatory as long as it was done quickly thereafter.

I'm uncomfortable with some of the bits inside the CachingMemorySSAWalker routines. Would you mind submitting the first version of the memory SSA code without them and then reviewing those separately? There's some very complex logic there that duplicates parts of MDA. Given how tricky MDA has been, I'm really not comfortable with the code in the walkers as it stands today without a much more careful review

lib/Transforms/Utils/MemorySSA.cpp
49 ↗(On Diff #23347)

Isn't this line redundant with the above?

79 ↗(On Diff #23347)

I'm skipping all the code copied from PMtR. I'm going to assume it's correct.

p.s. Any chance this could be expressed in a common template used by both places?

281 ↗(On Diff #23347)

Can we use smart pointers by chance? The manual memory management seems unnecessary.

303 ↗(On Diff #23347)

We no longer throw this out do we?

321 ↗(On Diff #23347)

This should be pulled out in a helper lambda or otherwise commoned.

327 ↗(On Diff #23347)

else if - once you do that, can't this just become:
if (def) {
} else if (use) {
} else {

llvm_unreachable()

}

415 ↗(On Diff #23347)

Don't you need to add these to the per-BB list? Also, can this code be commoned with the memory SSA construction path?

427 ↗(On Diff #23347)

It looks like you're not really using the instruction at all, maybe just pass the basicblock to begin with?

462 ↗(On Diff #23347)

Again, can we common this somewhere? You've got this lazy creation scatter all over the place.

Accesses = ensureBBList(BB)?

504 ↗(On Diff #23347)

This seems odd. Shouldn't we be putting Replacer in the spot Replacee was?

547 ↗(On Diff #23347)

Did you possible mean a != here?

560 ↗(On Diff #23347)

This condition really doesn't feel like a "replacement". Is it time to reconsider the interface and turn this into an assert?

571 ↗(On Diff #23347)

It might be cleaner to unconditionally insert the new value, then fold single use memory phis. Having a fold single use phi utility might also be useful on the public interface.

Actually, are you expecting an invariant where such phis are always folded? If so, please update comments to say so.

611 ↗(On Diff #23347)

If there are no uses, why is it required? Did you mean:
use_empty || onlySingleValue?

620 ↗(On Diff #23347)

You've got this code repeated a bit. Utility function on memoryPhi?

821 ↗(On Diff #23347)

Can't you just use cl opt for this?

852 ↗(On Diff #23347)

smart pointer?

942 ↗(On Diff #23347)

Er, huh? How can this happen? And should it?

1005 ↗(On Diff #23347)

Er, what? This just seems flat out wrong. Two loads on the same thread to the same location are always ordered regardless of flags. Ordering doesn't bypass a mustalias...

Unless this is absolutely required, I would request you separate this and review it separately.

1025 ↗(On Diff #23347)

I might be missing something here, but how is this not an infinite recursion on itself?

1042 ↗(On Diff #23347)

This seems suspicious to me as said above.

More generally, the smarts of this routine deserve to be reviewed independently.

1083 ↗(On Diff #23347)

Mild preference, change this to:
else if (CallSite CS = ...) {
} else {
}

1110 ↗(On Diff #23347)

Does this optimization actually help? Just curious.

Hi Daniel,

I'm looking at making LLVM recognize an idiom that I think MemorySSA+PRE would handle. What's the status of MemorySSA?

I know you haven't had much time to work on it recently; is it in such a state that someone (me) could help progress it?

Cheers,

James

Yes, it was looked at and approved, it just needs more tests written
and the initial update infrastructure ripped out and then slowly
rebuilt.
I can send you the latest patch, which fixes a bunch of bugs/etc.

hfinkel edited edge metadata.Dec 13 2015, 2:04 AM

Yes, it was looked at and approved, it just needs more tests written
and the initial update infrastructure ripped out and then slowly
rebuilt.
I can send you the latest patch, which fixes a bunch of bugs/etc.

Can you please post it here too, so we can continue the review?

pete added a subscriber: pete.Dec 13 2015, 9:32 PM
anemet added a subscriber: anemet.Dec 14 2015, 1:27 PM
dberlin edited edge metadata.

Refactor and rewrite a lot of stuff. This is somewhat of an intermediate state, so it's a bit messier than i'd like.
This version builds but i haven't played with it in a few months, so i expect it is bitrotted and may be a bit broken.
I am updating it mainly to give someone else a chance to takeover since i'm going to be swamped for a while.

Main algorithmic changes:
We now use BFS to find nearest dominating access in the caching walker.
I've performed extensive performance testing of hybrid DFS and BFS schemes, and BFS wins every time.
A bunch of things have started being iteratorized.
The caching walker has been modified a bit. The current caching scheme is fast, but can't easily be invalidated on a per-access basis.
The overhead of simple schemes that supported that level of invalidation was *quite* large.
(the ideal caching storage scheme here is actually quadtrees).

The caching walker now uses the same phi translation mechanism that memorydepedenceanalysis does.
I'm not sure this is a good thing, as that phi translation mechanism is ... strange and weird and has weird optimizations embedded in it.
It may be better to get worse optimization to start and clean that stuff up.

Updating D7864: This patch introduces MemorySSA, a virtual SSA form for memory.

Details on what it looks like are in MemorySSA.h

Also note: BFS or a hybrid DFS scheme is required to actually fulfill the
API guarantee we make - that we will give you the nearest dominating access.
DFS may find an access that is dominating but not the nearest dominating
one, depending on what paths it follows.

Thanks for the update!

Quick comment (just so I don't forget), there are a few places that have:

assert(N < 10000 && ..)

that need to become actual conservative bailouts instead of asserts.

lib/Transforms/Utils/MemorySSA.cpp
1284 ↗(On Diff #43896)

You might also want to check AA->pointsToConstantMemory here.

Yeah, these were just debugging asserts. They are there so i got asserts
on infinite loops and could bugpoint the testcases.
There shouldn't be any bugs left (most of the infinite loops were due to
inconsistency between the iterator and phitransaddr in what the result of
phi translation was. Now that they use the same thing, there is no
inconsistency) :)

They should definitely be removed, and i've run memoryssa across google's
codebase without them triggering.

I'm pretty sure i won't get to this patch in the next 6 months, though i'm
trying to find someone on my side to take it over. If someone else wants
to, that'd be awesome as well.

dberlin updated this revision to Diff 44284.Jan 7 2016, 3:52 PM

Fix comments, remove DFS, update tests to work again (aliasing got too smart)

Updating D7864: This patch introduces MemorySSA, a virtual SSA form for memory.

Details on what it looks like are in MemorySSA.h

hfinkel added inline comments.Jan 7 2016, 4:27 PM
lib/Transforms/Utils/MemorySSA.cpp
1124 ↗(On Diff #43896)

This algorithm in getClobberingMemoryAccess does not seem correct. If we have this:

   [ entry ]
       |
      ...
       |
   (clobbering access - a)
      ...
  /          \
...         (clobbering access - b)
 |              |
...           ....
 \              /
        ...
 (starting access)

UpwardsBFSWalkAccess seems to walk upward until it finds its first clobbering access, and returns that when found. In the case above, if we find clobbering access (b) first, then we're okay, because we'll pick some dominating def farther down the path which is on all paths to both dominating accesses.

But, if we hit clobbering access (a) first, then we'll accept that (incorrectly) as the new final access, because it dominates the original. However, not all paths to all aliasing defs terminate (or pass through) that access.

I think that you can continue the walk so that you search the entire subgraph to make sure that all paths terminate at the same aliasing def, and only in that case, use it (and, in that case, you know it must dominate).

My attempts to think up a good solution to the more-general problem have thus-far been stymied by this situation:

[entry]
  |
.....
(clobbering access - b)
 |
....   ________________________________
  \   /                                                               |
   (x)                                                               |
 ......                                                               |
    |                                                                 |
    |    ______________________                 |
     \   /                                        |                   |
(starting access)                        |                   |
     ...                                           |                   |
 (clobbering access - a)              |                   |
    ...                                            |                   |
    | |                                            |                   |
    | |______________________|                   |
    |                                                                  |
    |_________________________________|

The idea is that the block with the starting access also later has a clobbering access (a). This block is one of its own predecessors, but that's not the only path from clobbering access (a) back to the starting access. And furthermore, the other path flows through other blocks that do properly dominate the starting access. Thus, even if you were to pick a def (or phi) on the path from the starting access through its dominating predecessor, say at (x), and even though there are paths to all aliasing defs along that route, you'd still be wrong because there exists a path to clobbering access (a) that does not contain (x) (the path following the inner-loop backedge).

Thoughts? (am I off base here?)

hliao added a subscriber: hliao.Jan 7 2016, 10:49 PM

In-progress update

This brings back the DFS code and starts to update the BFS code to do the right thing with phi nodes.
Given the structure of the SSA graph and our requirements, i'm pretty positive both DFS and BFS will give correct results (and testing them against each other bears this out, though is not exhaustive).

Given that, and how ugly the BFS code is now starting to become to support the phi node rules, i'm tempted to get rid of it and just stick with DFS.

Thoughts welcome :)

Updating D7864: This patch introduces MemorySSA, a virtual SSA form for memory.

Details on what it looks like are in MemorySSA.h

Remove BFS code, fix a bug in DFS code.

(Updating so gbiv can take over)

Updating D7864: This patch introduces MemorySSA, a virtual SSA form for memory.

Details on what it looks like are in MemorySSA.h

george.burgess.iv commandeered this revision.Feb 1 2016, 11:50 AM
george.burgess.iv added a reviewer: dberlin.

It turns out that *can* steal revisions -- yay.

Taking ownership, killing D16743

Added Danny's patch that makes everything a User, as well as all of the updates published originally in the now-abandoned D16743.

Addressed all feedback in D16743, as well.

dberlin edited edge metadata.Feb 1 2016, 2:46 PM

You need

static inline bool classof(const Value *V) {
<right stuff>
}

For each of the classes. I only pasted it into memorydef, but without it, walking over operands and casting uses will not work.

But other than that, LGTM, i don't really have anything to add.
Thanks for taking this on!

george.burgess.iv edited edge metadata.
george.burgess.iv marked 71 inline comments as done.
  • Marked dead feedback as done, responded to others, will respond to remaining non-done + unanswered by tomorrow
  • Fixed Danny's comment about having classof(Value*)
  • Moved initialize[...]Pass methods to fix a bug with how we were trying to link things
  • Ripped out some optimizations (e.g. checks for invariant loads, etc.) because those seem mildly controversial, and can be added in a later patch

Thanks for the LGTM, Danny.

First wave of responses...

include/llvm/Transforms/Utils/MemorySSA.h
133 ↗(On Diff #46563)

Because it breaks code like (ripped from MemoryDef::print(raw_ostream &)):

MemoryAccess *UO = getDefiningAccess();
if (UO && UO->getID()) // Breaks
  // ...

...Because protected doesn't let you call protected methods on other MemoryAccesses. You can call them on yourself, or you can call them on other MemoryDefs. That's all.

Do you think it would be better to make getID() public, or to keep the friend declarations? I'm happy with either.

151 ↗(On Diff #46563)

It wasn't entirely an isa relationship. It's now refactored to be more clear.

293 ↗(On Diff #46563)

Definition of MemoryPhi::setIncomingValue is now substantially smaller. Leaving in header, unless you'd still like to see it moved.

408 ↗(On Diff #46563)

Update API has been removed for now.

lib/Transforms/Utils/MemorySSA.cpp
80 ↗(On Diff #46563)

Comment is on code that seems to have been removed.

87 ↗(On Diff #46563)

Comment is on code that seems to have been removed.

152 ↗(On Diff #46563)

Comment is on code that seems to have been removed.

621 ↗(On Diff #46563)

Now only exists in one place AFAICT.

963 ↗(On Diff #46563)

Nuked possiblyAffectedBy from orbit; it's an optimization that can be readded later.

963 ↗(On Diff #46563)

Do you still hold this opinion? (Potentially dubious is now in CachingMemorySSAWalker::instructionClobbersQuery)

963 ↗(On Diff #46563)

Marked with a TODO so we can look into that when we have users of MemorySSA/a more complete API/...

963 ↗(On Diff #46563)

Seems old; we now use DFS. Marking as done, but added a test case of your second example.

963 ↗(On Diff #46563)

I think it's better to get MemorySSA in and work on little optimizations like this after that. Removed the invariant check (but kept the test as an XFAIL), to be readded in the near future as a set of enhancements.

george.burgess.iv marked 10 inline comments as done.Feb 1 2016, 5:36 PM

After further investigation, the rest of the comments seem to be dead/already addressed, too; marked them as done.

Also, wow. Responding to comments at the bottom *really* didn't go well. I thought it would reflow them, but apparently not. Sorry about that. :/

Either way, I'll leave this open for a bit, in case anyone has any other remarks. Assuming Danny's LGTM is sufficient to let me commit, I'll do so lateish Wednesday if there are no objections by then. :)

Trying to make the comment/response chain at the bottom look less confusing...

reames

Er, what? This just seems flat out wrong. Two loads on the same thread to the same location are always ordered regardless of flags. Ordering doesn't bypass a mustalias...
Unless this is absolutely required, I would request you separate this and review it separately.

Nuked possiblyAffectedBy from orbit; it's an optimization that can be readded later.

Does this optimization actually help? Just curious.

Marked with a TODO so we can look into that when we have users of MemorySSA/a more complete API/...

This seems suspicious to me as said above.
More generally, the smarts of this routine deserve to be reviewed independently.

possiblyAffectedBy was removed, so I think this comment doesn't apply anymore (nor does my initial response to it). :)

hfinkel

You might also want to check AA->pointsToConstantMemory here.

I think it's better to get MemorySSA in and work on little optimizations like this after that. Removed the invariant check (but kept the test as an XFAIL), to be readded in the near future as a set of enhancements. (Good point, though!)

This algorithm in getClobberingMemoryAccess does not seem correct [...]

Seems old; we now use DFS. Marking as done, but added a test case of your second example.

reames accepted this revision.Feb 1 2016, 5:44 PM
reames added a reviewer: reames.

Explicitly LGTM as well. I'm really looking forward to having this in tree so that we can start experimenting and iterating on top of it.

I don't think we're going to be ready to switch the default for a while yet, but being able to iterate and work through the bugs in tree is going to be very empowering and will likely speedup progress.

p.s. What's your planned next step? I'd suggest implementing a very simple pass which just runs obvious peepholes over the MemorySSA representation. (e.g. a MemCombine pass). My suspicion is that will be a good way to stress the implementation and might even be profitable enough to just turn on by default. In particular, a pass that got the easy forms of cross block DSE would be very useful..

This revision is now accepted and ready to land.Feb 1 2016, 5:44 PM

FWIW: My plan was going to start to replace some of the existing
memorydependence users which are O(N^2) and ugly with simpler O(N)
algorithms using MemorySSA

Explicitly LGTM as well. I'm really looking forward to having this in tree so that we can start experimenting and iterating on top of it.

Thanks! (Also, naturally, thanks to everyone who reviewed/commented/... :) )

I don't think we're going to be ready to switch the default for a while yet, but being able to iterate and work through the bugs in tree is going to be very empowering and will likely speedup progress.

It seems that there are a number of people who are concerned about subtle bugs (both in MemorySSA, and in user code -- especially if this lets us optimize code in new and exciting ways). So, I agree that making this opt-in for a time is a good idea.

What's your planned next step? I'd suggest implementing a very simple pass which just runs obvious peepholes over the MemorySSA representation. (e.g. a MemCombine pass). My suspicion is that will be a good way to stress the implementation and might even be profitable enough to just turn on by default. In particular, a pass that got the easy forms of cross block DSE would be very useful.

I don't have any plans. If we want to port existing passes soon, we'll either need an update API, or we'll need to restrict ourselves to passes that don't use the update API. Also, we'll need a wrapper that either queries MemDep or MemorySSA if we want to have a global "use MemorySSA?" flag. If we think making a simple MemorySSA-based DSE pass would be a better first step, I'm fine doing that, as well. Same thing goes for readding the optimizations that were stripped out today.

So long as progress is made, I'm happy. :)

Excellent progress.

As Danny mentioned, the O(N^2) behavior of some MemoryDependence users
are biting us badly in some cases. For instance, some of the C++
source takes forever to compile with profile instrumentation turned on
and the root cause is due to that. Replacing those with memSSA based
algorithms will have very large (good) impact.

thanks,

David

I don't have any plans. If we want to port existing passes soon, we'll
either need an update API, or we'll need to restrict ourselves to passes
that don't use the update API.

FWIW: For the passes i played with converting, timing compute memssa + use
it vs memdep, for basically all testcases i could find, the time difference
was in the noise.
For larger cases, memssa is a straight win.
This is because most of the memdep passes are walking a ton of blocks/insts
repeatedly anyway.

Also, we'll need a wrapper that either queries MemDep or MemorySSA if we
want to have a global "use MemorySSA?" flag.

I'm not sure such a flag makes sense, given the above. I'm not sure you
*want* to try to alternate between them, because a lot of your order of
magnitude speedups will come from doing things fundamentally different
than random querying (IE move to more "SSA-like" algorithms).

(Though i expect using memssa as a straight replacement will still likely
be faster).

If we think making a simple MemorySSA-based DSE pass would be a better

first step, I'm fine doing that, as well. Same thing goes for readding the
optimizations that were stripped out today.

If you do the optimizations, i'm happy to start to convert passes. I was
going to just do them in close to the order necessary to preserve memssa
from beginning to end of opts that use it, but happy to do whatever.

(mergedloadstoremotion and memcpyoptimizer are the easiest things to
convert and will give speedups).

Converting most passes requires building an update API, and converting a
few of them and seeing what they want out of updating is the best way i can
think of to design that API.

(You can see where i played with it in the mergedloadstoremotion mpass).

So long as progress is made, I'm happy. :)

http://reviews.llvm.org/D7864

This revision was automatically updated to reflect the committed changes.
jlebar added a subscriber: jlebar.Mar 1 2016, 5:55 PM

I'm seeing an asan failure in one of the tests added here. I'm building from tip, clean tree, linux x86-64.

-- Testing: 1 of 15992 tests, 1 threads --
FAIL: LLVM-Unit :: Transforms/Utils/UtilsTests/MemorySSA.RemoveMemoryAccess (1 of 1)
******************** TEST 'LLVM-Unit :: Transforms/Utils/UtilsTests/MemorySSA.RemoveMemoryAccess' FAILED ********************
Note: Google Test filter = MemorySSA.RemoveMemoryAccess
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from MemorySSA
[ RUN      ] MemorySSA.RemoveMemoryAccess
=================================================================
==5162==ERROR: AddressSanitizer: heap-use-after-free on address 0x60800000bb68 at pc 0x000000c96299 bp 0x7ffea8a84a90 sp 0x7ffea8a84a88
READ of size 8 at 0x60800000bb68 thread T0
  #0 0xc96298 in llvm::MemoryAccess::getBlock() const /usr/local/google/home/jlebar/llvm/src/include/llvm/Transforms/Utils/MemorySSA.h:116:34
  #1 0xc96298 in llvm::MemorySSA::removeFromLookups(llvm::MemoryAccess*) /usr/local/google/home/jlebar/llvm/src/lib/Transforms/Utils/MemorySSA.cpp:469
  #2 0x58ffee in MemorySSA_RemoveMemoryAccess_Test::TestBody() /usr/local/google/home/jlebar/llvm/src/unittests/Transforms/Utils/MemorySSA.cpp:65:3
  #3 0xd66ec3 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
  #4 0xd66ec3 in testing::Test::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2161
  #5 0xd6b854 in testing::TestInfo::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2309:5
  #6 0xd6cb16 in testing::TestCase::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2416:5
  #7 0xd85c7a in testing::internal::UnitTestImpl::RunAllTests() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:4207:11
  #8 0xd84eaf in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
  #9 0xd84eaf in testing::UnitTest::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:3841
  #10 0xd3cc67 in main /usr/local/google/home/jlebar/llvm/src/utils/unittest/UnitTestMain/TestMain.cpp:47:10
  #11 0x7fc886a0dec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287
  #12 0x54bd09 in _start (/usr/local/google/home/jlebar/code/llvm/asan/unittests/Transforms/Utils/UtilsTests+0x54bd09)

0x60800000bb68 is located 72 bytes inside of 96-byte region [0x60800000bb20,0x60800000bb80)
freed by thread T0 here:
  #0 0x4c734b in operator delete(void*) (/usr/local/google/home/jlebar/code/llvm/asan/unittests/Transforms/Utils/UtilsTests+0x4c734b)
  #1 0xc9602e in llvm::ilist_node_traits<llvm::MemoryAccess>::deleteNode(llvm::MemoryAccess*) /usr/local/google/home/jlebar/llvm/src/include/llvm/ADT/ilist.h:160:39
  #2 0xc9602e in llvm::iplist<llvm::MemoryAccess, llvm::ilist_traits<llvm::MemoryAccess> >::erase(llvm::ilist_iterator<llvm::MemoryAccess>) /usr/local/google/home/jlebar/llvm/src/include/llvm/ADT/ilist.h:466
  #3 0xc9602e in llvm::iplist<llvm::MemoryAccess, llvm::ilist_traits<llvm::MemoryAccess> >::erase(llvm::MemoryAccess*) /usr/local/google/home/jlebar/llvm/src/include/llvm/ADT/ilist.h:470
  #4 0xc9602e in llvm::MemorySSA::removeFromLookups(llvm::MemoryAccess*) /usr/local/google/home/jlebar/llvm/src/lib/Transforms/Utils/MemorySSA.cpp:467
  #5 0x58ffee in MemorySSA_RemoveMemoryAccess_Test::TestBody() /usr/local/google/home/jlebar/llvm/src/unittests/Transforms/Utils/MemorySSA.cpp:65:3
  #6 0xd66ec3 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
  #7 0xd66ec3 in testing::Test::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2161
  #8 0xd6b854 in testing::TestInfo::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2309:5
  #9 0xd6cb16 in testing::TestCase::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2416:5
  #10 0xd85c7a in testing::internal::UnitTestImpl::RunAllTests() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:4207:11
  #11 0xd84eaf in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
  #12 0xd84eaf in testing::UnitTest::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:3841
  #13 0xd3cc67 in main /usr/local/google/home/jlebar/llvm/src/utils/unittest/UnitTestMain/TestMain.cpp:47:10
  #14 0x7fc886a0dec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287

previously allocated by thread T0 here:
  #0 0x4c6e0b in operator new(unsigned long) (/usr/local/google/home/jlebar/code/llvm/asan/unittests/Transforms/Utils/UtilsTests+0x4c6e0b)
  #1 0xa7ec05 in llvm::User::allocateFixedOperandUser(unsigned long, unsigned int, unsigned int) /usr/local/google/home/jlebar/llvm/src/lib/IR/User.cpp:129:7
  #2 0xa7ec05 in llvm::User::operator new(unsigned long, unsigned int) /usr/local/google/home/jlebar/llvm/src/lib/IR/User.cpp:147
  #3 0xc92784 in llvm::MemoryDef::operator new(unsigned long) /usr/local/google/home/jlebar/llvm/src/include/llvm/Transforms/Utils/MemorySSA.h:271:41
  #4 0xc92784 in llvm::MemorySSA::createNewAccess(llvm::Instruction*, bool) /usr/local/google/home/jlebar/llvm/src/lib/Transforms/Utils/MemorySSA.cpp:378
  #5 0xc9098f in llvm::MemorySSA::buildMemorySSA(llvm::AAResults*, llvm::DominatorTree*) /usr/local/google/home/jlebar/llvm/src/lib/Transforms/Utils/MemorySSA.cpp:260:26
  #6 0x58f91b in MemorySSA_RemoveMemoryAccess_Test::TestBody() /usr/local/google/home/jlebar/llvm/src/unittests/Transforms/Utils/MemorySSA.cpp:55:29
  #7 0xd66ec3 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
  #8 0xd66ec3 in testing::Test::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2161
  #9 0xd6b854 in testing::TestInfo::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2309:5
  #10 0xd6cb16 in testing::TestCase::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2416:5
  #11 0xd85c7a in testing::internal::UnitTestImpl::RunAllTests() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:4207:11
  #12 0xd84eaf in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
  #13 0xd84eaf in testing::UnitTest::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:3841
  #14 0xd3cc67 in main /usr/local/google/home/jlebar/llvm/src/utils/unittest/UnitTestMain/TestMain.cpp:47:10
  #15 0x7fc886a0dec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287

SUMMARY: AddressSanitizer: heap-use-after-free /usr/local/google/home/jlebar/llvm/src/include/llvm/Transforms/Utils/MemorySSA.h:116 llvm::MemoryAccess::getBlock() const
Shadow bytes around the buggy address:
0x0c107fff9710: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c107fff9720: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c107fff9730: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c107fff9740: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c107fff9750: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 fa
=>0x0c107fff9760: fa fa fa fa fd fd fd fd fd fd fd fd fd[fd]fd fd
0x0c107fff9770: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
0x0c107fff9780: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 fa
0x0c107fff9790: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 fa
0x0c107fff97a0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 fa
0x0c107fff97b0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 03 fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable:           00
Partially addressable: 01 02 03 04 05 06 07 
Heap left redzone:       fa
Heap right redzone:      fb
Freed heap region:       fd
Stack left redzone:      f1
Stack mid redzone:       f2
Stack right redzone:     f3
Stack partial redzone:   f4
Stack after return:      f5
Stack use after scope:   f8
Global redzone:          f9
Global init order:       f6
Poisoned by user:        f7
Container overflow:      fc
ASan internal:           fe
==5162==ABORTING

********************
Testing Time: 0.56s
********************
Failing Tests (1):
  LLVM-Unit :: Transforms/Utils/UtilsTests/MemorySSA.RemoveMemoryAccess

Unexpected Failures: 1

Thanks for the report -- looking into it now :)

It seems that the test in question wasn't introduced by this patch, but the fix seems somewhat trivial, so my attempt was checked in as r262452. Please let me know if that didn't end up fixing it (I lack an ASAN-instrumented build of LLVM :) ).

Please let me know if that didn't end up fixing it (I lack an ASAN-instrumented build of LLVM :) ).

Works now, although now that it's fixed asan is showing me memory leaks. :) One seems to be in the test itself, but another appears to be in the actual code.

Clearly not as urgent, but here's the report:

********************
FAIL: LLVM-Unit :: Transforms/Utils/UtilsTests/MemorySSA.RemoveMemoryAccess (15985 of 15994)
******************** TEST 'LLVM-Unit :: Transforms/Utils/UtilsTests/MemorySSA.RemoveMemoryAccess' FAILED ********************
Note: Google Test filter = MemorySSA.RemoveMemoryAccess
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from MemorySSA
[ RUN      ] MemorySSA.RemoveMemoryAccess
[       OK ] MemorySSA.RemoveMemoryAccess (25 ms)
[----------] 1 test from MemorySSA (25 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (26 ms total)
[  PASSED  ] 1 test.

=================================================================
==147403==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 304 byte(s) in 1 object(s) allocated from:
    #0 0x4c66ab in operator new(unsigned long) (/usr/local/google/home/jlebar/code/llvm/asan/unittests/Transforms/Utils/UtilsTests+0x4c66ab)
    #1 0xc89da5 in llvm::MemorySSA::buildMemorySSA(llvm::AAResults*, llvm::DominatorTree*) /usr/local/google/home/jlebar/llvm/src/lib/Transforms/Utils/MemorySSA.cpp:234:3
    #2 0x58f1eb in MemorySSA_RemoveMemoryAccess_Test::TestBody() /usr/local/google/home/jlebar/llvm/src/unittests/Transforms/Utils/MemorySSA.cpp:55:29
    #3 0xd60843 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
    #4 0xd60843 in testing::Test::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2161
    #5 0xd651d4 in testing::TestInfo::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2309:5
    #6 0xd66496 in testing::TestCase::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2416:5
    #7 0xd7f5fa in testing::internal::UnitTestImpl::RunAllTests() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:4207:11
    #8 0xd7e82f in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
    #9 0xd7e82f in testing::UnitTest::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:3841
    #10 0xd365e7 in main /usr/local/google/home/jlebar/llvm/src/utils/unittest/UnitTestMain/TestMain.cpp:47:10
    #11 0x7fb091525ec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287

Indirect leak of 1024 byte(s) in 1 object(s) allocated from:
    #0 0x4c66ab in operator new(unsigned long) (/usr/local/google/home/jlebar/code/llvm/asan/unittests/Transforms/Utils/UtilsTests+0x4c66ab)
    #1 0x58e7b8 in MemorySSA_RemoveMemoryAccess_Test::TestBody() /usr/local/google/home/jlebar/llvm/src/unittests/Transforms/Utils/MemorySSA.cpp:53:3
    #2 0xd60843 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
    #3 0xd60843 in testing::Test::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2161
    #4 0xd651d4 in testing::TestInfo::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2309:5
    #5 0xd66496 in testing::TestCase::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2416:5
    #6 0xd7f5fa in testing::internal::UnitTestImpl::RunAllTests() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:4207:11
    #7 0xd7e82f in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
    #8 0xd7e82f in testing::UnitTest::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:3841
    #9 0xd365e7 in main /usr/local/google/home/jlebar/llvm/src/utils/unittest/UnitTestMain/TestMain.cpp:47:10
    #10 0x7fb091525ec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287

ndirect leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x4c66ab in operator new(unsigned long) (/usr/local/google/home/jlebar/code/llvm/asan/unittests/Transforms/Utils/UtilsTests+0x4c66ab)
    #1 0x58e777 in MemorySSA_RemoveMemoryAccess_Test::TestBody() /usr/local/google/home/jlebar/llvm/src/unittests/Transforms/Utils/MemorySSA.cpp:52:3
    #2 0xd60843 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
    #3 0xd60843 in testing::Test::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2161
    #4 0xd651d4 in testing::TestInfo::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2309:5
    #5 0xd66496 in testing::TestCase::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2416:5
    #6 0xd7f5fa in testing::internal::UnitTestImpl::RunAllTests() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:4207:11
    #7 0xd7e82f in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
    #8 0xd7e82f in testing::UnitTest::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:3841
    #9 0xd365e7 in main /usr/local/google/home/jlebar/llvm/src/utils/unittest/UnitTestMain/TestMain.cpp:47:10
    #10 0x7fb091525ec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287

Indirect leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x4c66ab in operator new(unsigned long) (/usr/local/google/home/jlebar/code/llvm/asan/unittests/Transforms/Utils/UtilsTests+0x4c66ab)
    #1 0x58ee7f in void llvm::AAResults::addAAResult<llvm::BasicAAResult>(llvm::BasicAAResult&) /usr/local/google/home/jlebar/llvm/src/include/llvm/Analysis/AliasAnalysis.h:173:5
    #2 0x58ee7f in MemorySSA_RemoveMemoryAccess_Test::TestBody() /usr/local/google/home/jlebar/llvm/src/unittests/Transforms/Utils/MemorySSA.cpp:54
    #3 0xd60843 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
    #4 0xd60843 in testing::Test::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2161
    #5 0xd651d4 in testing::TestInfo::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2309:5
    #6 0xd66496 in testing::TestCase::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2416:5
    #7 0xd7f5fa in testing::internal::UnitTestImpl::RunAllTests() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:4207:11
    #8 0xd7e82f in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
    #9 0xd7e82f in testing::UnitTest::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:3841
    #10 0xd365e7 in main /usr/local/google/home/jlebar/llvm/src/utils/unittest/UnitTestMain/TestMain.cpp:47:10
    #11 0x7fb091525ec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287

Indirect leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x4c66ab in operator new(unsigned long) (/usr/local/google/home/jlebar/code/llvm/asan/unittests/Transforms/Utils/UtilsTests+0x4c66ab)
    #1 0x592596 in __gnu_cxx::new_allocator<std::unique_ptr<llvm::AAResults::Concept, std::default_delete<llvm::AAResults::Concept> > >::allocate(unsigned long, void const*) /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/ext/new_allocator.h:104:27
    #2 0x592596 in std::_Vector_base<std::unique_ptr<llvm::AAResults::Concept, std::default_delete<llvm::AAResults::Concept> >, std::allocator<std::unique_ptr<llvm::AAResults::Concept, std::default_delete<llvm::AAResults::Concept> > > >::_M_allocate(unsigned long) /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_vector.h:168
    #3 0x592596 in void std::vector<std::unique_ptr<llvm::AAResults::Concept, std::default_delete<llvm::AAResults::Concept> >, std::allocator<std::unique_ptr<llvm::AAResults::Concept, std::default_delete<
llvm::AAResults::Concept> > > >::_M_emplace_back_aux<llvm::AAResults::Model<llvm::BasicAAResult>*>(llvm::AAResults::Model<llvm::BasicAAResult>*&&) /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/vector.tcc:404
    #4 0x58f1cd in void std::vector<std::unique_ptr<llvm::AAResults::Concept, std::default_delete<llvm::AAResults::Concept> >, std::allocator<std::unique_ptr<llvm::AAResults::Concept, std::default_delete<
llvm::AAResults::Concept> > > >::emplace_back<llvm::AAResults::Model<llvm::BasicAAResult>*>(llvm::AAResults::Model<llvm::BasicAAResult>*&&) /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/vector.tcc:101:4
    #5 0x58f1cd in void llvm::AAResults::addAAResult<llvm::BasicAAResult>(llvm::BasicAAResult&) /usr/local/google/home/jlebar/llvm/src/include/llvm/Analysis/AliasAnalysis.h:173
    #6 0x58f1cd in MemorySSA_RemoveMemoryAccess_Test::TestBody() /usr/local/google/home/jlebar/llvm/src/unittests/Transforms/Utils/MemorySSA.cpp:54
    #7 0xd60843 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
    #8 0xd60843 in testing::Test::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2161
    #9 0xd651d4 in testing::TestInfo::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2309:5
    #10 0xd66496 in testing::TestCase::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2416:5
    #11 0xd7f5fa in testing::internal::UnitTestImpl::RunAllTests() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:4207:11
    #12 0xd7e82f in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:2145:12
    #13 0xd7e82f in testing::UnitTest::Run() /usr/local/google/home/jlebar/llvm/src/utils/unittest/googletest/src/gtest.cc:3841
    #14 0xd365e7 in main /usr/local/google/home/jlebar/llvm/src/utils/unittest/UnitTestMain/TestMain.cpp:47:10
    #15 0x7fb091525ec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287

SUMMARY: AddressSanitizer: 1384 byte(s) leaked in 5 allocation(s).

Actually, sigh, we didn't do that already because we hand them back to the
user as well.
Anyway, i'm working up a patch to fix these.

This should hopefully fix all the leaks and asan errors (there is one real
bug here).
It is asan clean on OSX now, i am building on linux to do the leak check.