[PATCH] D18065: Fix bugs in the MemorySSA walker

[Daniel Berlin via llvm-commits]

Yeah, a great example


entry:

if (a) goto foo
else goto exit

foo:
goto foo

exit:
....


foo is forward reachable, but reverse unreachable.


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

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