[PATCH] D24805: [GVNSink] Initial GVNSink prototype

James Molloy via llvm-commits llvm-commits at lists.llvm.org
Thu May 25 08:43:22 PDT 2017


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.

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<mailto:dberlin at dberlin.org>> wrote:



On Thu, May 25, 2017 at 8:24 AM, James Molloy <James.Molloy at arm.com<mailto: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<mailto: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<mailto: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/b2356d18/attachment.html>


More information about the llvm-commits mailing list