RE: [RFC, vectorizer] Allow single element vector types for vector reduction operations

[Richard Biener <rguenther@suse.de>]

On Wed, 30 Aug 2017, Jon Beniston wrote:

> Hi Richard,
> 
> >> Ah, that's what I first tried, and mistakenly sent the patch against
> that.
> >> 
> >> That did help the initial problem, but then I needed to add a lot of 
> >> support for the V1SI type in the backend (which just duplicated SI 
> >> patterns) and there are a few places where GCC seems to have sanity 
> >> checks against vectors that are only one element wide. A dot product 
> >> producing a scalar result seems natural.
> >
> >Where do you need V1SI types?  The vectorizer should perform a SI extract 
> >from V1SI here.  You need to elaborate a bit here.
> 
> After adding single element vector type support in the middle-end, at the
> tree level, V1SI vector types would be the result type for the dot product
> if the input operands were V2HI.
> 
> During RTL expansion, if V1SImode is accepted in the
> TARGET_VECTOR_MODE_SUPPORTED_P hook, then V1SImode will be used after RTL
> expansion and V1SImode patterns are needed, although they are actually
> duplicates of SImode patterns.  I have now removed V1SImode from
> TARGET_VECTOR_MODE_SUPPORTED_P, and they are turned into SI mode so there is
> no need for the V1SI patterns now.
> 
> >> The vectorizer doesn't really support vector->scalar ops so V1SI 
> >> feels more natural here.  That is, your patch looks a bit ugly -- 
> >> there's nothing wrong with V1SI vector types.
> 
> I agree "there's nothing wrong with V1SI vector types" and the original
> patch was trying to let vector reduction operation accept V1SI vector types.
> 
> However, if the only change was "<= 1" into "< 1" in
> get_vectype_for_scalar_type_and_size, then it will cause a GCC assertion
> failure in optab_for_tree_node for some shift operations. It seems all such
> failures come from:
> 
> vect_pattern_recog_1:4185
>       optab = optab_for_tree_code (code, type_in, optab_default);
> 
> The SUB_TYPE passed to optab_for_tree_code is "optab_default", however it is
> possible that vect_pattern_recog_1 will generate gimple containing shift
> operations, for example vect_recog_divmod_pattern will generate RSHIFT_EXPR,
> while shift operation requires sub_type to be not optab_default?

I think the issue is that with TARGET_VECTOR_MODE_SUPPORTED_P false
for V1SI you'll get a SImode vector type and the

  if (VECTOR_BOOLEAN_TYPE_P (type_in)
      || VECTOR_MODE_P (TYPE_MODE (type_in)))

check fails.  Try changing that to || VECTOR_TYPE_P (type_in).
The else path should be hopefully dead ...

With your TARGET_VECTOR_MODE_SUPPORTED_P setting you are essentially
making the vectorizer mix what was desired as "generic" vectorization
and regular vectorization...  if it works, fine ;)

>   gcc/optabs-tree.h:
> 
>   /* An extra flag to control optab_for_tree_code's behavior.  This is
> needed to
>      distinguish between machines with a vector shift that takes a scalar
> for the
>      shift amount vs. machines that take a vector for the shift amount.  */
> 
>   enum optab_subtype
>   {
>     optab_default,
>     optab_scalar,
>     optab_vector
>   };
> 
> It looks to me like the other call sites of optab_for_tree_code which are
> passing optab_default are mostly places where GCC is sure it is not a shift
> operation.
> Given TYPE_IN is returned from get_vectype_for_scalar_type, is it safe to
> simply pass optab_vector in vect_pattern_recog_1?

I guess so.  Can you also try the above?

> I have verified the following middle-end fix also works for my port, it
> passed also x86-64 bootstrap and there is no regression in gcc/g++
> regression.
> 
> How is this new patch?
> 
> gcc/
> 2017-08-30  Jon Beniston  <jon@beniston.com>
> 
>         * tree-vect-patterns.c (vect_pattern_recog_1): Pass optab_vector
> when
>         calling optab_for_tree_code.
>         * tree-vect-stmts.c (get_vectype_for_scalar_type_and_size): Allow
> single
>         element vector types.
> 
> diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
> index cfdb72c..4b39cb6 100644
> --- a/gcc/tree-vect-patterns.c
> +++ b/gcc/tree-vect-patterns.c
> @@ -4182,7 +4182,7 @@ vect_pattern_recog_1 (vect_recog_func *recog_func,
>           code = CALL_EXPR;
>         }
> 
> -      optab = optab_for_tree_code (code, type_in, optab_default);
> +      optab = optab_for_tree_code (code, type_in, optab_vector);
>        vec_mode = TYPE_MODE (type_in);
>        if (!optab
>            || (icode = optab_handler (optab, vec_mode)) == CODE_FOR_nothing
> diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
> index 013fb1f..fc62efb 100644
> --- a/gcc/tree-vect-stmts.c
> +++ b/gcc/tree-vect-stmts.c
> @@ -9098,7 +9098,8 @@ get_vectype_for_scalar_type_and_size (tree
> scalar_type, unsigned size)
>    else
>      simd_mode = mode_for_vector (inner_mode, size / nbytes);
>    nunits = GET_MODE_SIZE (simd_mode) / nbytes;
> -  if (nunits <= 1)
> +  /* NOTE: nunits == 1 is allowed to support single element vector types.
> */
> +  if (nunits < 1)
>      return NULL_TREE;
> 
>    vectype = build_vector_type (scalar_type, nunits);