[PATCH] D24805: [GVNSink] Initial GVNSink prototype

[Daniel Berlin via llvm-commits]

On Thu, May 25, 2017, 8:43 AM James Molloy <James.Molloy at arm.com> wrote:

> PHIs are lowered by inserting COPYs into each predecessor. Switches only
> want to generate control flow, nothing else, so the theoretical insertion
> point for the COPY is above the switch.
>

Usually edges are split if necessary.


> It would be possible to do what you suggest by manually splitting the
> switch up into its constituent compares (producing a binary search tree)
> with each leaf jumping to the same destination. But in that case each leaf
> is in a different basic block so you don’t have duplicate arcs any more :)
>
> On 25 May 2017, at 16:36, Daniel Berlin <dberlin at dberlin.org> wrote:
>
>
>
> On Thu, May 25, 2017 at 8:24 AM, James Molloy <James.Molloy at arm.com>
> wrote:
>
>> [+my non-arm account]
>>
>> Hi,
>>
>> I believe I have a testcase for that too. The key is the word
>> “conflicting” - we can have multiple edges from the same predecessor, but
>> to be well formed all phi values for those arcs must be the same. The
>> verifier checks this, if I recall.
>>
>
> Interesting.  I guess that is because it doesn't know how to map them back
> to the real edges or something.
>
> IE:
>  define i32 @f3(i32 %x) {
>  bb0:
>    switch i32 %x, label %bb1 [
> i32 0, label %bb2
> i32 1, label %bb2
> ]
>  bb1:
>    br label %bb2
>  bb2:
>    %cond = phi i32 [ 0, %bb1 ], [ 0, %bb0 ], [ 1, %bb0 ]
>    %foo = add i32 %cond, %x
>    ret i32 %foo
>  }
>
>
> IE %cond = phi i32 [ 0, %bb1 ], [ 0, %bb0 ], [ 1, %bb0 ]
>
> should be valid, but you are right, it is not, LLVM requires it to be
>
>  %cond = phi i32 [ 0, %bb1 ], [ %x, %bb0 ], [ %x, %bb0 ]
>
> (In the above, i know i could eliminate the add along two paths if i could
> make a phi node with different values for each of the edges)
>
>
> I’ll make sure there’s a testcase at least for the full expected behaviour
>> (this pass is off by default at the moment so there’s no panic about trunk
>> miscompiling).
>>
>> James
>>
>> On 25 May 2017, at 16:11, Daniel Berlin <dberlin at dberlin.org> wrote:
>>
>> (That comment applies mostly to newgvn).
>>
>> In your case, i believe the code is just wrong:
>>
>> / This assumes the PHI is already well-formed and there aren't conflicting
>>     // incoming values for the same block.
>>     for (auto *B : Blocks)
>>       Values.push_back(PN->getIncomingValueForBlock(B))
>>
>> It's 100% completely and definitely possible for a phi to have two values
>> incoming from the same block, actually, regardless of whether it's a self
>> block or not.
>> This happens using switch statements.
>> I believe we even have some testcases in newgvn for multiple edges from
>> same incoming block.
>>
>> So yeah, you have to handle them here.
>>
>>
>> On Thu, May 25, 2017 at 8:08 AM, Daniel Berlin <dberlin at dberlin.org>
>> wrote:
>>
>>>
>>>> ----------------
>>>> dberlin wrote:
>>>> > I'm curious why you think you need stable sort here as opposed to
>>>> regular
>>>> >
>>>> I don't. Changed.
>>>>
>>>>
>>> (i'm offsite today, but someone should test this in newgvn too if i'm
>>> right).
>>>
>>> Do we allow switch statements with multiple edges to ourself?
>>>
>>> ie
>>>
>>> bb1:
>>>
>>> switch <whatever> [
>>> i32 0 : label bb1
>>> i32 1: label bb1 ]
>>>
>>>
>>> (which, after propagation,  could cause a phi with different operands
>>> and the same incoming blocks)
>>> If so, either we need stable sorts, or a better ordering of incoming
>>> blocks, because the pointer equality we use will not definitely sort them
>>> into a consistent order
>>>
>>>
>>
>>
>> IMPORTANT NOTICE: The contents of this email and any attachments are
>> confidential and may also be privileged. If you are not the intended
>> recipient, please notify the sender immediately and do not disclose the
>> contents to any other person, use it for any purpose, or store or copy the
>> information in any medium. Thank you.
>>
>
>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170525/b989426d/attachment.html>