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

[Ehsan A Amiri via llvm-commits]

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



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/20160713/17442563/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/20160713/17442563/attachment.gif>