On Fri, 2018-08-17 at 16:00 +0200, Richard Biener wrote:
> On Wed, Aug 8, 2018 at 12:59 AM Segher Boessenkool
> <seg...@kernel.crashing.org> wrote:
> >
> > Hi!
> >
> > On Tue, Aug 07, 2018 at 02:24:58PM -0500, Will Schmidt wrote:
> > >    This adds support for gimple folding of vec_mergeh and vec_mergel
> > > for float and double types.   Support for the integral types is already
> > > in-tree.
> >
> > > +  /* The permute_type will match the lhs for integral types.  For double 
> > > and
> > > +     float types, the permute type needs to map to the V2 or V4 type that
> > > +     matches size.  */
> > > +  tree permute_type;
> > > +  if (INTEGRAL_TYPE_P (TREE_TYPE (lhs_type)))
> > > +    permute_type = lhs_type;
> > > +  else
> > > +    if (TREE_TYPE (lhs_type) == TREE_TYPE (V2DF_type_node))
> > > +      permute_type = V2DI_type_node;
> > > +    else if (TREE_TYPE (lhs_type) == TREE_TYPE (V4SF_type_node))
> > > +      permute_type = V4SI_type_node;
> > > +    else
> > > +      gcc_unreachable ();
> >
> > Please write this as
> >
> >   if (INTEGRAL_TYPE_P (TREE_TYPE (lhs_type)))
> >     permute_type = lhs_type;
> >   else if (TREE_TYPE (lhs_type) == TREE_TYPE (V2DF_type_node))
> >     permute_type = V2DI_type_node;
> >   else if (TREE_TYPE (lhs_type) == TREE_TYPE (V4SF_type_node))
> >     permute_type = V4SI_type_node;
> >   else
> >     gcc_unreachable ();
> >
> > or, if you want to emphasize integer vs. float:
> >
> >   if (INTEGRAL_TYPE_P (TREE_TYPE (lhs_type)))
> >     permute_type = lhs_type;
> >   else
> >     {
> >       if (TREE_TYPE (lhs_type) == TREE_TYPE (V2DF_type_node))
> 
> Are you sure lhs_type is never qualified?

For the V2DF and V4SF types, I am mostly sure, but since you bring it up
I will admit I am not positive.  :-)       

>                                     That is, for a GIMPLE folder
> I'd have expected
>
>   if (types_compatible_p (TREE_TYPE (lhs_type), TREE_TYPE (V2DF_type_node)))

> for GENERIC
> 
>   if (TYPE_MAIN_VARIANT (TREE_TYPE (lhs_type)) == TREE_TYPE (V2DF_type_node))

Either/both of those seem more robust than what I had come up with, I'll
plan to make an update here.

Any guidance on whether I should prefer "types_compatible_p" versus the
GENERIC "TYPE_MAIN_VARIANT" ?   I see more references to the latter in
rs6000.c , but the former seems to make better sense to me just by its
name.

Thanks for the review, 
-Will


> Richard.
> 
> >         permute_type = V2DI_type_node;
> >       else if (TREE_TYPE (lhs_type) == TREE_TYPE (V4SF_type_node))
> >         permute_type = V4SI_type_node;
> >       else
> >         gcc_unreachable ();
> >     }
> >
> > Okay for trunk with that changed.  Thanks!
> >
> >
> > Segher
> 


Reply via email to