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

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

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 :)

> For now, let's fix this NewGVN bug though.
> 
>> I'm under the impression the problem is that we don't take in consideration the incoming edges for congruency. This patch makes sure we only consider congruent phis if the incoming BBs are the same.
> 
> This is the wrong way to do this.
>  The right way, if we have to, is to always sort them into the same operand order :)

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

> IE sort the operands into pred order before adding them.
> 
> It will actually just suffice to sort them in pointer order of the incoming blocks.
>  The phi nodes of a given bb must have the same incoming blocks.
>  So the only thing one must do to put them in that order is to sort the incoming blocks into the same order.
> 
> It should suffice to do this:
>  push all operands (use's) into a small vector
>  sort by getIncomingBlock(Use)
>  (getIncomingBlock(Use) is constant time)
> 
> Add to phi expression in that order.


https://reviews.llvm.org/D32990