Hi all,

These patches refactor the widening patterns in vect-patterns to use internal_fn instead of tree_codes.

Sorry about the delay, some changes to master made it a bit messier.

Bootstrapped and regression tested on aarch64.

Joel

> > diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c index
> > 854cbcff390..4a8ea67e62f 100644
> > --- a/gcc/tree-vect-patterns.c
> > +++ b/gcc/tree-vect-patterns.c
> > @@ -1245,7 +1245,7 @@ vect_recog_sad_pattern (vec_info *vinfo,  static
> > gimple *  vect_recog_widen_op_pattern (vec_info *vinfo,
> >  			     stmt_vec_info last_stmt_info, tree *type_out,
> > -			     tree_code orig_code, tree_code wide_code,
> > +			     tree_code orig_code, code_helper
> wide_code_or_ifn,
> 
> I think it'd be better to keep the original “wide_code” name and try to
> remove as many places as possible in which switching based on tree_code
> or internal_fn is necessary.  The recent gimple-match.h patches should
> help with that, but more routines might be needed.

Done.

> > @@ -1309,8 +1310,16 @@ vect_recog_widen_op_pattern (vec_info *vinfo,
> >  		       2, oprnd, half_type, unprom, vectype);
> >
> >    tree var = vect_recog_temp_ssa_var (itype, NULL);
> > -  gimple *pattern_stmt = gimple_build_assign (var, wide_code,
> > -					      oprnd[0], oprnd[1]);
> > +  gimple *pattern_stmt;
> > +  if (wide_code_or_ifn.is_tree_code ())
> > +    pattern_stmt = gimple_build_assign (var, wide_code_or_ifn,
> > +						oprnd[0], oprnd[1]);
> > +  else
> > +    {
> > +      internal_fn fn = as_internal_fn ((combined_fn) wide_code_or_ifn);
> > +      pattern_stmt = gimple_build_call_internal (fn, 2, oprnd[0], oprnd[1]);
> > +      gimple_call_set_lhs (pattern_stmt, var);
> > +    }
> 
> For example, I think we should hide this inside a new:
> 
>   gimple_build (var, wide_code, oprnd[0], oprnd[1]);
> 
> that works directly on code_helper, similarly to the new code_helper
> gimple_build interfaces.

Done.

> > @@ -4513,14 +4513,16 @@ vect_gen_widened_results_half (vec_info
> *vinfo, enum tree_code code,
> >    tree new_temp;
> >
> >    /* Generate half of the widened result:  */
> > -  gcc_assert (op_type == TREE_CODE_LENGTH (code));
> >    if (op_type != binary_op)
> >      vec_oprnd1 = NULL;
> > -  new_stmt = gimple_build_assign (vec_dest, code, vec_oprnd0,
> vec_oprnd1);
> > +  if (ch.is_tree_code ())
> > +    new_stmt = gimple_build_assign (vec_dest, ch, vec_oprnd0,
> vec_oprnd1);
> > +  else
> > +    new_stmt = gimple_build_call_internal (as_internal_fn ((combined_fn)
> ch),
> > +					   2, vec_oprnd0, vec_oprnd1);
> 
> Similarly here.  I guess the combined_fn/internal_fn path will also need
> to cope with null trailing operands, for consistency with the tree_code one.
> 

Done.

> > @@ -4744,31 +4747,28 @@ vectorizable_conversion (vec_info *vinfo,
> >        && ! vec_stmt)
> >      return false;
> >
> > -  gassign *stmt = dyn_cast <gassign *> (stmt_info->stmt);
> > -  if (!stmt)
> > +  gimple* stmt = stmt_info->stmt;
> > +  if (!(is_gimple_assign (stmt) || is_gimple_call (stmt)))
> >      return false;
> >
> > -  if (TREE_CODE (gimple_assign_lhs (stmt)) != SSA_NAME)
> > -    return false;
> > +  if (is_gimple_assign (stmt))
> > +  {
> > +    code_or_ifn = gimple_assign_rhs_code (stmt);
> > +  }
> > +  else
> > +    code_or_ifn = gimple_call_combined_fn (stmt);
> 
> It might be possible to use gimple_extract_op here (only recently added).
> This would also provide the number of operands directly, instead of
> needing “op_type”.  It would also provide an array of operands.
> 

Done.

> > -  code = gimple_assign_rhs_code (stmt);
> > -  if (!CONVERT_EXPR_CODE_P (code)
> > -      && code != FIX_TRUNC_EXPR
> > -      && code != FLOAT_EXPR
> > -      && code != WIDEN_PLUS_EXPR
> > -      && code != WIDEN_MINUS_EXPR
> > -      && code != WIDEN_MULT_EXPR
> > -      && code != WIDEN_LSHIFT_EXPR)
> 
> Is it safe to drop this check independently of parts 2 and 3?  (Genuine
> question, haven't checked in detail.)

It requires the parts 2 and 3. I've moved that change into this first patch.

> > @@ -4784,7 +4784,8 @@ vectorizable_conversion (vec_info *vinfo,
> >      }
> >
> >    rhs_type = TREE_TYPE (op0);
> > -  if ((code != FIX_TRUNC_EXPR && code != FLOAT_EXPR)
> > +  if ((code_or_ifn.is_tree_code () && code_or_ifn != FIX_TRUNC_EXPR
> > +       && code_or_ifn != FLOAT_EXPR)
> 
> I don't think we want the is_tree_code condition here.  The existing
> != should work.
> 

Done.

> > @@ -11856,13 +11888,13 @@ supportable_widening_operation (vec_info
> *vinfo,
> >    if (BYTES_BIG_ENDIAN && c1 != VEC_WIDEN_MULT_EVEN_EXPR)
> >      std::swap (c1, c2);
> >
> > -  if (code == FIX_TRUNC_EXPR)
> > +  if (code_or_ifn == FIX_TRUNC_EXPR)
> >      {
> >        /* The signedness is determined from output operand.  */
> >        optab1 = optab_for_tree_code (c1, vectype_out, optab_default);
> >        optab2 = optab_for_tree_code (c2, vectype_out, optab_default);
> >      }
> > -  else if (CONVERT_EXPR_CODE_P (code)
> > +  else if (CONVERT_EXPR_CODE_P ((tree_code) code_or_ifn)
> 
> I think this should be as_tree_code (), so that it's safe for internal
> functions if (tree_code) ever becomes a checked convrsion in future.
> Same for other instances.
> 

Done.

> >  	   && VECTOR_BOOLEAN_TYPE_P (wide_vectype)
> >  	   && VECTOR_BOOLEAN_TYPE_P (vectype)
> >  	   && TYPE_MODE (wide_vectype) == TYPE_MODE (vectype)
> > […]
> > @@ -12000,7 +12031,7 @@ supportable_widening_operation (vec_info
> *vinfo,
> >  bool
> >  supportable_narrowing_operation (enum tree_code code,
> >  				 tree vectype_out, tree vectype_in,
> > -				 enum tree_code *code1, int *multi_step_cvt,
> > +				 void* _code1, int *multi_step_cvt,
> 
> This might be rehashing an old conversation, sorry, but why does this
> need to be void?
> 

Reworked to avoid using void*.

> >                                   vec<tree> *interm_types)
> >  {
> >    machine_mode vec_mode;
> > @@ -12013,6 +12044,7 @@ supportable_narrowing_operation (enum
> tree_code code,
> >    machine_mode intermediate_mode, prev_mode;
> >    int i;
> >    bool uns;
> > +  tree_code * code1 = (tree_code*) _code1;
> >
> >    *multi_step_cvt = 0;
> >    switch (code)
> > diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
> > index bd6f334d15f..70c06264c11 100644
> > --- a/gcc/tree-vectorizer.h
> > +++ b/gcc/tree-vectorizer.h
> > @@ -2030,13 +2030,16 @@ extern bool vect_is_simple_use (vec_info *,
> stmt_vec_info, slp_tree,
> >  				enum vect_def_type *,
> >  				tree *, stmt_vec_info * = NULL);
> >  extern bool vect_maybe_update_slp_op_vectype (slp_tree, tree);
> > -extern bool supportable_widening_operation (vec_info *,
> > -					    enum tree_code, stmt_vec_info,
> > -					    tree, tree, enum tree_code *,
> > -					    enum tree_code *, int *,
> > -					    vec<tree> *);
> > +extern bool supportable_widening_operation (vec_info *vinfo,
> > +				code_helper code_or_ifn,
> > +				stmt_vec_info stmt_info,
> > +				tree vectype_out, tree vectype_in,
> > +				code_helper *code_or_ifn1,
> > +				code_helper *code_or_ifn2,
> > +				int *multi_step_cvt,
> > +				vec<tree> *interm_types);
> 
> Normal style is to keep the variable names out of the header.
> The documentation lives in the .c file, so in practice, anyone
> who wants to add a new caller will need to look there anyway.
> 
> Thanks,
> Richard
> 
> >  extern bool supportable_narrowing_operation (enum tree_code, tree,
> tree,
> > -					     enum tree_code *, int *,
> > +					     void *, int *,
> >  					     vec<tree> *);
> >
> >  extern unsigned record_stmt_cost (stmt_vector_for_cost *, int,
> > diff --git a/gcc/tree.h b/gcc/tree.h
> > index f62c00bc870..346565f84ce 100644
> > --- a/gcc/tree.h
> > +++ b/gcc/tree.h
> > @@ -6546,5 +6546,31 @@ extern unsigned fndecl_dealloc_argno (tree);
> >     if nonnull, set the second argument to the referenced enclosing
> >     object or pointer.  Otherwise return null.  */
> >  extern tree get_attr_nonstring_decl (tree, tree * = NULL);
> > +/* Helper to transparently allow tree codes and builtin function codes
> > +   exist in one storage entity.  */
> > +class code_helper
> > +{
> > +public:
> > +  code_helper () {}
> > +  code_helper (tree_code code) : rep ((int) code) {}
> > +  code_helper (combined_fn fn) : rep (-(int) fn) {}
> > +  operator tree_code () const { return is_tree_code () ?
> > +						       (tree_code) rep :
> > +						       ERROR_MARK; }
> > +  operator combined_fn () const { return is_fn_code () ?
> > +						       (combined_fn) -rep:
> > +						       CFN_LAST; }
> > +  bool is_tree_code () const { return rep > 0; }
> > +  bool is_fn_code () const { return rep < 0; }
> > +  int get_rep () const { return rep; }
> > +
> > +  enum tree_code as_tree_code () const { return is_tree_code () ?
> > +    (tree_code)* this : MAX_TREE_CODES; }
> > +  combined_fn as_fn_code () const { return is_fn_code () ? (combined_fn)
> *this
> > +    : CFN_LAST;}
> > +
> > +private:
> > +  int rep;
> > +};
> >
> >  #endif  /* GCC_TREE_H  */