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

[Ehsan A Amiri via llvm-commits]

This is order of events:

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.
3) GVN makes a mem-dep query for dependencies of a load instruction. (Let's
denote it with A)
4) memdep encounters a store insn (let's denote it with B).
5) memdep performs an alias analysis query for pointer operands of A and B.
6) memdep is told that the pointer operands of A and B are aliased. (They
are not really aliased, but BasicAA can not see through the phi node)
7) memdep now thinks that B clobbers A. This information is cached.
8) GVN is told that B clobbers A. GVN gives up on removing A.
9) GVN removes the phi node.
10) GVN tries again to remove A. It queries memdep.
11) memdep looks at its cache. It finds that B clobbers A. It returns that
information to GVN.
12) GVN gives up again and does not touch A.

Now if mem-dep cache was working properly,in step 11 we would have made a
new alias analysis and then prove that B does not clobber A and A would
have been removed in step 12.

Given the complexity of fixing the real problem, if alias analysis can see
through phi, we can prove that pointer operands of A and B are not aliased
in steps 5 and 6, so memdep never returns  or cache a conservative result.

Thanks
Ehsan




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



So now I'm even more confused. If it's using the memdep cache, why does
changing basicaa work at all?
Should it not still get the cached info

On Wed, Jul 13, 2016, 8:22 PM Ehsan A Amiri <amehsan at ca.ibm.com> wrote:
  Daniel,

  The soultion you are proposing does not work. Actually it is already
  implemented and we have this problem. Very early in processInstruction,
  there is a call to SimplifyInstruction, which provides that exact same
  functionality for PHI nodes and we still have this problem.

  This goes back to what we already discussed in detail in the PR. Actually
  GVN does not directly make an Alias Analysis query. It makes a MemDep
  query. At the first iteration of GVN, when thet mem-dep does not have the
  results cached (and so it does all the work, which includes alias
  analysis query), the phi exists. After the phi is removed, the cache
  already contains invalid information, so memdep does not make a new alias
  analysis query. It just provides us with its invalid cache information.
  So we are back at our previous discussion :)

  Thanks
  Ehsan

  Inactive hide details for Daniel Berlin ---07/13/2016 09:34:56 PM---On
  Wed, Jul 13, 2016 at 5:55 PM, Ehsan Amiri <amehsan at ca.ibDaniel Berlin
  ---07/13/2016 09:34:56 PM---On Wed, Jul 13, 2016 at 5:55 PM, Ehsan Amiri
  <amehsan at ca.ibm.com> wrote: > amehsan added a comment.

  From: Daniel Berlin <dberlin at dberlin.org>
  To: reviews+D22305+public+92ca108e50bc4651 at reviews.llvm.org
  Cc: Ehsan A Amiri/Toronto/IBM at IBMCA, Hal Finkel <hfinkel at anl.gov>,
  llvm-commits <llvm-commits at lists.llvm.org>
  Date: 07/13/2016 09:34 PM
  Subject: Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming
  values are the same.








  On Wed, Jul 13, 2016 at 5:55 PM, Ehsan Amiri <amehsan at ca.ibm.com> wrote:


        amehsan added a comment.

        @dberlin

        I see two concerns in your comment:

        > > Why not fix GVN to do that?

        >

        >

        >


        Before GVN this phi node has two different incoming values. At some
        point A in GVN, one of the incoming values change and we get the
        phi node in the example above. At some point B in the GVN, the phi
        node is removed. Somewhere between A and B, GVN makes an alias
        analysis query. So I think there is not really a problem in GVN to
        fix.

  I'm going to pretty strongly disagree.  If GVN expects AA to give it good
  intermediate answers, it needs to give AA good intermediate code.

  GVN does a lot of simplification already. In fact, this would probably be
  one of the cheapest things GVN does It seems 100% trivial to make this
  work, unless you can show me it won't :)

  Change:

     for (BasicBlock::iterator BI = BB->begin(), BE = BB->end();
          BI != BE;) {
       if (!ReplaceWithConstMap.empty())
         ChangedFunction |= replaceOperandsWithConsts(&*BI);
       ChangedFunction |= processInstruction(&*BI);

  To have a processPhiNode call at the beginning.
  In processPhiNode, if all arguments are the same, RAUW with the phi node
  result.

  Or heck, throw it in processInstruction

  Now, if that doesn't work, or you have actual inter-pass examples where
  this is getting left around, sure, fine.
  I want to avoid useless checks in alias analysis (and elsewhere) for
  non-canonical code.

  Look at it another way:

  If this is common IR, every analysis is likely to need to do something
  with it, at some cost.

  It's much cheaper to *not* have to process it at all, than have to
  process it everywhere.






-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160714/6ecb0a59/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/6ecb0a59/attachment.gif>