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

Thu Jul 14 14:46:07 PDT 2016

Yes, IIUC that is from an inlined function that has GVN run on it earlier.
all those pre variables has "i" suffix which I believe are created by
inlining.

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:38 PM
Subject:	Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all
            incoming values are the same.

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 )

  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>
  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.

        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> wrote:

                    On Thu, Jul 14, 2016 at 7:09 AM, Ehsan A Amiri <
                    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/0cbca0bb/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/0cbca0bb/attachment.gif>