[PATCH] D18065: Fix bugs in the MemorySSA walker

[Daniel Berlin via llvm-commits]

On Thu, Mar 10, 2016 at 7:16 PM, George Burgess IV <
george.burgess.iv at gmail.com> wrote:

> 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,
>
> It's possible for blocks to go nowhere.

There is no guarantee they ever reach an exit :)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160310/6655f38c/attachment.html>