[PATCH] D18065: Fix bugs in the MemorySSA walker

[George Burgess IV via llvm-commits]

george.burgess.iv added inline comments.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:970
@@ +969,3 @@
+      // a valid construct, we still need to handle it, because users can ask
+      // the walker to walk arbitrary IR.
+      ModifyingAccess = FirstDef ? FirstDef : MSSA->getLiveOnEntryDef();
----------------
dberlin wrote:
> Errr.
> What?
> 
> A. In buildMemorySSA, we already detect forward-unreachable-from-entry blocks and mark all definitions as live on entry. So they should never loop anywhere in the case you are listing, because if the loop has no entry, all the defs/uses in it should be to live on entry.
> 
> So i don't see how this can happen.
>  
> However
> B.
> Because we already handle forward unreachable-from-entry blocks and mark all defs in them as liveonentry in buildmemoryssa,  is there a good reason we shouldn't just detect reverse unreachable-from-entry blocks and do the same?
> 
> We can say that MemorySSA has no forward or reverse unreachable blocks, and make the IR sane in them. This is because we have no real IR, and so, at worst, we can do what we do now:
> 
> Mark anything in unreachable as live on entry, and remove all phis in unreachable.  We could go one more, and remove all the memory accesses entirely if we wanted to.
> 
> 
> A. In buildMemorySSA, we already detect forward-unreachable-from-entry blocks and mark all definitions as live on entry. So they should never loop anywhere in the case you are listing, because if the loop has no entry, all the defs/uses in it should be to live on entry.

Correct.

> So i don't see how this can happen.

I was thinking that the update API might be able to get us in this situation, but after looking at it, I think I was wrong. :)

Ternary replaced with `assert(FirstDef);`

> Because we already handle forward unreachable-from-entry blocks and mark all defs in them as liveonentry in buildmemoryssa, is there a good reason we shouldn't just detect reverse unreachable-from-entry blocks and do the same?

I can't think of one. Though, I'm also not sure I understand backwards reachability, because I can't think of a case where a reverse unreachable-from-entry block would be walked by a DFS of the domtree. Would a trivial algorithm for determining reverse reachability-from-entry for some node N not be "flip the edges of the CFG, and see if Entry is reachable from N"?

> Mark anything in unreachable as live on entry, and remove all phis in unreachable. We could go one more, and remove all the memory accesses entirely if we wanted to.

Would it be fine to just not build them in the first place?


http://reviews.llvm.org/D18065