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

Hal Finkel via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 13 18:14:12 PDT 2016


hfinkel added a comment.

In http://reviews.llvm.org/D22305#483557, @amehsan wrote:

> @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.


On the one hand, we generally don't construct our analyses to deal with trivially-non-canonical IR. One could argue that if GVN expects to use analyses on its intermediate states, then those states need to follow the rules too. On the other hand, arbitrary uses of RAUW can result in these kinds of PHIs, so this is not really a GVN problem, and handling this reduces our phase-ordering sensitivities. While several passes will clean these up, the same is true for several other things that stripPointerCasts handles.

> 

> 

> > > IE if we next discover cases where all the phi arguments are redundant

> 

> > 

> 

> > >  bitcasts of the same value, do we add that?

> 

> > 

> 

> > >  What about more complex operations?

> 

> > 

> 

> > 

> 

> > 

> 

> 

> I think eventually, this code should be part of stripPointerCasts that we call in lines 1403-1404. There we handle pointers that are hidden behind bitcasts and GEPs where indices are zero.


I agree. If we're going to do this at all, the code should be added to stripPointerCasts. I don't see a good reason to add it here. Please split out the functional change from the performance-related change, and put the functional change in stripPointerCasts so it can be discussed separately.


http://reviews.llvm.org/D22305





More information about the llvm-commits mailing list