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

[Daniel Berlin via llvm-commits]

Note: This already had GVN run once on it, do you have the one before that?


On Thu, Jul 14, 2016 at 1:53 PM, Ehsan A Amiri <amehsan at ca.ibm.com> wrote:

> I can see the problem with the following command line and attached file
>
> opt -gvn t.ll
>
> *(See attached file: t.ll)*
>
> My clang is almost a week old.
>
> [image: Inactive hide details for Daniel Berlin ---07/14/2016 04:27:43
> PM---Actually, can you please attach a .ll file and an opt comma]Daniel
> Berlin ---07/14/2016 04:27:43 PM---Actually, can you please attach a .ll
> file and an opt command line that reproduces the problem?
>
> From: Daniel Berlin <dberlin at dberlin.org>
> To: Ehsan A Amiri/Toronto/IBM at IBMCA
> Cc: Hal Finkel <hfinkel at anl.gov>, llvm-commits <
> llvm-commits at lists.llvm.org>,
> reviews+D22305+public+92ca108e50bc4651 at reviews.llvm.org
> Date: 07/14/2016 04:27 PM
> Subject: Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming
> values are the same.
> ------------------------------
>
>
>
> Actually, can you please attach a .ll file and an opt command line that
> reproduces the problem?
> The clang command line you have is very sensitive to versions, etc.
>
> I cannot get your issue to reproduce with the clang i have installed that
> can target powerpc-linux (and the issue does not reproduce with your
> testcase on x86) :)
>
> While debugging a bit, note that there is at least one obvious bug in GVN
> that may affect this, by inspection:
>
> When GVN splits a critical edge, it never adds the new block to the
> iteration order (at all), even though it inserts into it.
> So they will not get processed until the next iteration of GVN on the
> function, even though they have code in them.  While this is okay from a
> correctness standpoint, it may block optimization of certain things
> (including the cases you've discovered).  In practice, there is no way to
> perfectly solve that without pre-splitting all critical edges, but you
> should get the same effect if we throw the critical edge block and then
> it's successors (including the current blcok) into bbvect after the current
> block again.
>
> It is also missing a real phi simplification.
> While simplifyinstruction will check if all arguments are trivially the
> same, that is not the real test that should be performed.
>
> It should be doing VN.lookup on each argument and seeing if they come up
> with the the same value number.
>
> Once you attach the .ll file, i'll fix these and see if it fixes your
> testcase, and if not, debug further.
>
>
> On Thu, Jul 14, 2016 at 10:11 AM, Daniel Berlin <*dberlin at dberlin.org*
> <dberlin at dberlin.org>> wrote:
>
>
>
>    On Thu, Jul 14, 2016 at 7:09 AM, Ehsan A Amiri <*amehsan at ca.ibm.com*
>    <amehsan at ca.ibm.com>> wrote:
>    This is order of events:
>
>    This order cannot be correct if the solution I gave (or adding
>    simplification to *some place in GVN*) does not work.  Or GVN is broken in
>    other ways.
>
>    1) GVN starts looking at the function. At this point the phi node has
>    two different incoming values.
>    2) GVN performs an RAUW. The phi is converted to the one that has two
>    identincal incoming values.
>
>
>    At this point, it should now process the phi instruction again before
>    it processes the load, because it is doing a reverse postorder traversal.
>    When it did that, the phi should have been simplified
>    So why did that not happen?
>
>    Given the complexity of fixing the real problem,
>
>
>    Look, i understand why you want to just fix this in AA and be done
>    with it.
>    Really, I do.
>
>    I understand you have spent a lot of time on this bug, and I greatly
>    appreciate that.
>    But I really want to understand what is going on before we try to
>    actually fix it.
>    I have a good understanding of what happens once the bad answer gets
>    into memdep (and thank you for that!), but i still have trouble seeing why
>    it lived long enough to get there.
>
>    To that end, so you don't have to spend more time running around for
>    me,  i'll take over this bug, and either figure out why GVN lets this PHI
>    live to the point it gets an AA query about it (and fix it/decide it can't
>    be fixed), or commit the AA patch for you if we decide it can't be fixed.
>
>    My ETA is by friday.
>
>    I assume the testcase in the bug is the one we are still using, right?
>    (If not, if you can attach it, that would be helpful)
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160714/f40ee52f/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/20160714/f40ee52f/attachment.gif>