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

[Ehsan A Amiri via llvm-commits]

Sure, I will attach it shortly.



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/956d52a9/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/956d52a9/attachment.gif>