[PATCH] D18065: Fix bugs in the MemorySSA walker

[George Burgess IV via llvm-commits]

TIL. That makes sense then -- thanks.
On Mar 10, 2016 7:18 PM, "Daniel Berlin" <dberlin at dberlin.org> wrote:

>
>
> 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/b1b5df7e/attachment.html>