Re: [patch] Fix PR tree-optimization/46049 and tree-optimization/46052

[Richard Guenther <rguenther at suse dot de>]

On Tue, 19 Oct 2010, Ira Rosen wrote:

> Richard Guenther <rguenther@suse.de> wrote on 19/10/2010 02:27:27 PM:
> 
> > > > Hm, but then this is a problem of the caller, and likely non-SLP mode
> > > > has the same issue (looking at vectorizable_operation I don't see
> > > > any special casing handling it for non-SLP, it'll just call
> > > > vect_get_vec_defs and then vect_get_vec_def_for_operand).
> > >
> > > vect_get_vec_def_for_operand actually treats constants and invariants
> > > differently, using number of units of the stmt's vectype for constants
> and
> > > the type of the operand for invariants.
> >
> > Well, as far as I can see vect_is_simple_use will assign the operands
> > defining statement definition to *def (which will be always op itself).
> > So,
> >
> >     case vect_constant_def:
> >       {
> >         vector_type = get_vectype_for_scalar_type (TREE_TYPE (op));
> >
> > and
> >
> >     case vect_external_def:
> >       {
> >         vector_type = get_vectype_for_scalar_type (TREE_TYPE (def));
> >
> > are the same.
> 
> Right, but the number of elements of created vector is determined by
> nunits, and for case vect_constant_def we use:
> 
>   tree vectype = STMT_VINFO_VECTYPE (stmt_vinfo);
>   unsigned int nunits = TYPE_VECTOR_SUBPARTS (vectype);
> 
> while
> 
>     case vect_external_def:
>       {
>         vector_type = get_vectype_for_scalar_type (TREE_TYPE (def));
>         gcc_assert (vector_type);
>         nunits = TYPE_VECTOR_SUBPARTS (vector_type);

Oh indeed.  A strange inconsistency.  But of course if
nunits != TYPE_VECTOR_SUBPARTS (vector_type) then we're generating
a bogus vector_cst anyway, so I'd argue the vect_external_def handling
is correct and the vect_constant_def one wrong.

> > > > I think that vectorizable_operation has to special case shifts
> > > > (it already does for the vectorization test but not for code
> generation),
> > > > which also means that it really asks for being separated out
> > > > into a vectorizable_shift function ...
> > > >
> > > > Basically the
> > > >
> > > >           if (op_type == binary_op && scalar_shift_arg)
> > > >             {
> > > >
> > > > case should also handle the variant where we need to extend
> > > > the scalar shift arg to a vector.
> > >
> > > But scalar_shift_arg indicates whether the shift argument is a scalar
> or a
> > > vector. How can we treat it as "we need to extend the scalar shift arg
> to a
> > > vector" as well?
> >
> > If the target doesn't support a non-vector shift amount then we always
> > need to promote it to a vector (and we know how many elements that
> > will have from the other operands element count).
> 
> But in that case scalar_shift_arg is false.

Oh, indeed.  So we need to always promote the arg to a vector then
(if !scalar_shift_arg).

> >
> > > vectorizable_shift sounds good to me, I hope there are no other
> operations
> > > like that, allowing operands of different types.
> >
> > I don't think so.
> 
> OK, good. I'll prepare a patch then.

Thanks,
Richard.