[PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming values are the same.

[Daniel Berlin via llvm-commits]

On Fri, Jul 15, 2016 at 1:34 PM, Ehsan A Amiri <amehsan at ca.ibm.com> wrote:

> So I will first experiment with (3). If we don't like the outcome, then we
> can experiment with (2) to see how feasible is that.
>
> *Daniel*: Thanks for the response to my question. But now I need to ask
> two more question to understand things:
>
> *As mentioned, in your testcase, there is another bug here.  Remember
>  that GVN iterates until things stop changing. So *the first gvn pass*
> should have never left the IR with that redundant PHI node in the first
> place.*
>
> 1- IIUC, we are saying that at the end of one GVN iteration, the IR should
> satisfy some properties. Is it possible to describe this properties?
>

Close. I mean "at the end of executing a  GVN pass, the IR should satisfy
some properties" (iterations are internal to gvn, and in the current GVN,
it may take multiple iterations to satisfy these properties).

The properties it should satisfy are[1]

1. Every value (V) dominated by another provably equivalent value (Leader)
should have V replaced by Leader.  This is "equivalence without taking into
account conditionals", and not even complete equivalence, but "equivalence
that this GVN algorithm can detect".  The GVN algorithm used is not a
complete one, so it misses plenty of provably equivalences.

As a trivial example:

for (int i =0, j = 0; i < 50; ++i, ++j)
{
int x = i + 2;
int y = j + 2;
A[x] = y
int z = A[y];
}

i, j, x, y, z are all equivalent.
Current GVN will detect none of these. This is not fixable.

NewGVN is not complete, but it will detect all of these.
Complete algorithms will detect all that can be detected :)

2. Every instruction should be simplified as done by SimplifyInst. If
simplification produces a value, all uses should be replaced with that
simplified value

[1] There are PRE and conditional propagation properties, but they are
harder to describe.

Now, for *each iteration*, there are invariants. The above is an iteration
level invariant except for the case where it does PRE and inserts code.  In
the case it does PRE and inserts code, it may not meet the above conditions.





> 2- I suspect that the "other bug" that you are talking about is a GVN bug.
> Could you elaborate on that if this is correct?
>
Yes.


> IIRC from my debugging, the redundant phi node was created and deleted in
> iteration 0 of GVN.
>
 I do not see this happen in some cases where i modify your testcase.
Again i was not being precise enough, and meant more "things your testcase
exposes after debugging the code".



> It was just that cache issue that happened between the creation and
> deletion of redundant phi node that caused this....
>
> Thanks
> Ehsan
>
>
>
> [image: Inactive hide details for Daniel Berlin ---07/15/2016 04:08:45
> PM---> >]Daniel Berlin ---07/15/2016 04:08:45 PM---> >
>
> From: Daniel Berlin <dberlin at dberlin.org>
> To: Hal Finkel <hfinkel at anl.gov>
> Cc: llvm-commits <llvm-commits at lists.llvm.org>,
> reviews+D22305+public+92ca108e50bc4651 at reviews.llvm.org, Ehsan A
> Amiri/Toronto/IBM at IBMCA
> Date: 07/15/2016 04:08 PM
> Subject: Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming
> values are the same.
> ------------------------------
>
>
>
>
>
>    Or do we need (2) because in general this kind of phi node is seen in
>    many programs? Or something else that I do not see....
>
>
>    That is hal's view.
>
>
>    My view was that we should measure the impact of the change and then
>    decide; but that if we have a use case for it, I'd rather it be inside the
>    generic utility than a specific work-around in BasicAA.
>
>
> Yes, sorry, was writing quickly, did not mean to imply anything else ;)
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160715/0042e878/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160715/0042e878/attachment.gif>