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

Thu Jul 14 14:37:58 PDT 2016

This still has a pre-phi named variable in it, which means GVN has been run
on it once (GVN is the only thing in llvm that will create variables with
names that have pre-phi in them) :)

In any case, i've debugged this enough on the existing file, i'll send an
update in a second.

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

> *(See attached file: t.ll)*
> This one should be correct. (Previous one seems to be something from the
> middle of GVN :P )
>
> [image: Inactive hide details for Daniel Berlin ---07/14/2016 05:12:47
> PM---Note: This already had GVN run once on it, do you have the]Daniel
> Berlin ---07/14/2016 05:12:47 PM---Note: This already had GVN run once on
> it, do you have the one before that? On Thu, Jul 14, 2016 at
>
> 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 05:12 PM
>
> Subject: Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming
> values are the same.
> ------------------------------
>
>
>
> 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*
> <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* <dberlin at dberlin.org>>
>    To: Ehsan A Amiri/Toronto/IBM at IBMCA
>    Cc: Hal Finkel <*hfinkel at anl.gov* <hfinkel at anl.gov>>, llvm-commits <
>    *llvm-commits at lists.llvm.org* <llvm-commits at lists.llvm.org>>,
>    *reviews+D22305+public+92ca108e50bc4651 at reviews.llvm.org*
>    <reviews%2BD22305%2Bpublic%2B92ca108e50bc4651 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/4c3d263f/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/4c3d263f/attachment.gif>