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

[Daniel Berlin via llvm-commits]

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/27aefb0d/attachment.html>