[PATCH] D32990: [NewGVN] Take in account incoming edges computing congruent PhiExpression(s)

Daniel Berlin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon May 8 22:03:45 PDT 2017


dberlin added a comment.

In https://reviews.llvm.org/D32990#749367, @davide wrote:

> In https://reviews.llvm.org/D32990#749362, @dberlin wrote:
>
> > > I may be completely off on this one, please bear with me :)
> > >  The way we currently define congruency for two PHIExpression(s) is:
> > > 
> > > 1. The operands to the phi functions are congruent
> > > 2. The PHIs are defined in the same Basic Block.
> >
> > This is correct. We assume operand order is always the same between phis in the same block.
> >
> > > The attached testcase contains the following:
> > > 
> > >   patatino:
> > >     %meh = phi i16 [ %0, %winky ], [ %conv1, %tinky ]
> > >     %banana = phi i16 [ %0, %tinky ], [ %conv1, %winky ]
> > >     br label %end
> > >    
> > >    
> >
> > So, NewGVN assumes the operands are in the order of predecessors to the block.  Or at least, a consistent order.
> >  I believed LLVM always puts them that way.
> >  I'm not sure we are even the only thing that assumes this.
> >  But it's honestly pretty silly to not have that invariant (Otherwise, you have to linearly search phi nodes to find edges from predecessors).
> >
> > Let's assume it's not true. We should probably make it true.  I have trouble seeing a downside.  I'm not sure where you would be messing up the edges separately from the phi nodes in a sane way ....
>
>
> I thought so too. I have a C program that generates this that I'll post/examine independently. I agree it's silly not having a consistent order but this is what we have right now :)


Sigh.

The only reason we have separate phi edge arrays at all right now is because predecessors are slow to access.

(Removing phi arguments in the middle is already not constant time right now anyway, see the comment in removeIncomingValue. We are already doing the thing we would have to do if we didn't store the phi edges)

Otherwise, they are a complete waste of space and time; it would be faster/better to always have them in pred order and make PhiNode->getIncomingBlock(index) == PhiNode->getParent()->getPred(index).

> Yes, my bad, this makes much more sense. I'll update the patch.

Thanks!


https://reviews.llvm.org/D32990





More information about the llvm-commits mailing list