[PATCH] D24805: [GVNSink] Initial GVNSink prototype

[Daniel Berlin via llvm-commits]

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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170525/3c92feae/attachment.html>