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

[Ehsan A Amiri via llvm-commits]

(See attached file: t.ll)
This one should be correct. (Previous one seems to be something from the
middle of GVN :P )



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 commaDaniel
  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/7097d488/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/7097d488/attachment.gif>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: t.ll
Type: application/octet-stream
Size: 4653 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160714/7097d488/attachment.obj>