RE: [ping][vect-patterns] Refactor widen_plus/widen_minus as internal_fns

[Richard Biener <rguenther@suse.de>]

On Tue, 7 Jun 2022, Joel Hutton wrote:

> Thanks Richard,
> 
> > I thought the potential problem with the above is that gimple_build is a
> > folding interface, so in principle it's allowed to return an existing SSA_NAME
> > set by an existing statement (or even a constant).
> > I think in this context we do need to force a new statement to be created.
> 
> Before I make any changes, I'd like to check we're all on the same page.
> 
> richi, are you ok with the gimple_build function, perhaps with a 
> different name if you are concerned with overloading? we could use 
> gimple_ch_build or gimple_code_helper_build?

We can go with a private vect_gimple_build function until we sort out
the API issue to unblock Tamar (I'll reply to Richards reply with further 
thoughts on this)

> Similarly are you ok with the use of gimple_extract_op? I would lean towards using it as it is cleaner, but I don't have strong feelings.

I don't like using gimple_extract_op here, I think I outlined a variant
that is even shorter.

Richard.

> Joel
> 
> > -----Original Message-----
> > From: Richard Sandiford <richard.sandiford@arm.com>
> > Sent: 07 June 2022 09:18
> > To: Joel Hutton <Joel.Hutton@arm.com>
> > Cc: Richard Biener <rguenther@suse.de>; gcc-patches@gcc.gnu.org
> > Subject: Re: [ping][vect-patterns] Refactor widen_plus/widen_minus as
> > internal_fns
> > 
> > Joel Hutton <Joel.Hutton@arm.com> writes:
> > >> > Patches attached. They already incorporated the .cc rename, now
> > >> > rebased to be after the change to tree.h
> > >>
> > >> @@ -1412,8 +1412,7 @@ 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 = gimple_build (var, wide_code, oprnd[0],
> > >> oprnd[1]);
> > >>
> > >>
> > >> you should be able to do without the new gimple_build overload by
> > >> using
> > >>
> > >>    gimple_seq stmts = NULL;
> > >>    gimple_build (&stmts, wide_code, itype, oprnd[0], oprnd[1]);
> > >>    gimple *pattern_stmt = gimple_seq_last_stmt (stmts);
> > >>
> > >> because 'gimple_build' is an existing API.
> > >
> > > Done.
> > >
> > > The gimple_build overload was at the request of Richard Sandiford, I
> > assume removing it is ok with you Richard S?
> > > From Richard Sandiford:
> > >> 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.
> > 
> > I thought the potential problem with the above is that gimple_build is a
> > folding interface, so in principle it's allowed to return an existing SSA_NAME
> > set by an existing statement (or even a constant).
> > I think in this context we do need to force a new statement to be created.
> > 
> > Of course, the hope is that there wouldn't still be such folding opportunities
> > at this stage, but I don't think it's guaranteed (especially with options
> > fuzzing).
> > 
> > Sind I was mentioned :-) ...
> > 
> > Could you run the patch through contrib/check_GNU_style.py?
> > There seem to be a few long lines.
> > 
> > > +  if (res_op.code.is_tree_code ())
> > 
> > Do you need this is_tree_code ()?  These comparisons…
> > 
> > > +  {
> > > +      widen_arith = (code == WIDEN_PLUS_EXPR
> > > +		     || code == WIDEN_MINUS_EXPR
> > > +		     || code == WIDEN_MULT_EXPR
> > > +		     || code == WIDEN_LSHIFT_EXPR);
> > 
> > …ought to be safe unconditionally.
> > 
> > > + }
> > > +  else
> > > +      widen_arith = false;
> > > +
> > > +  if (!widen_arith
> > > +      && !CONVERT_EXPR_CODE_P (code)
> > > +      && code != FIX_TRUNC_EXPR
> > > +      && code != FLOAT_EXPR)
> > > +    return false;
> > >
> > >    /* Check types of lhs and rhs.  */
> > > -  scalar_dest = gimple_assign_lhs (stmt);
> > > +  scalar_dest = gimple_get_lhs (stmt);
> > >    lhs_type = TREE_TYPE (scalar_dest);
> > >    vectype_out = STMT_VINFO_VECTYPE (stmt_info);
> > >
> > > @@ -4938,10 +4951,14 @@ vectorizable_conversion (vec_info *vinfo,
> > >
> > >    if (op_type == binary_op)
> > >      {
> > > -      gcc_assert (code == WIDEN_MULT_EXPR || code ==
> > WIDEN_LSHIFT_EXPR
> > > -		  || code == WIDEN_PLUS_EXPR || code ==
> > WIDEN_MINUS_EXPR);
> > > +      gcc_assert (code == WIDEN_MULT_EXPR
> > > +		  || code == WIDEN_LSHIFT_EXPR
> > > +		  || code == WIDEN_PLUS_EXPR
> > > +		  || code == WIDEN_MINUS_EXPR);
> > >
> > > -      op1 = gimple_assign_rhs2 (stmt);
> > > +
> > > +      op1 = is_gimple_assign (stmt) ? gimple_assign_rhs2 (stmt) :
> > > +				     gimple_call_arg (stmt, 0);
> > >        tree vectype1_in;
> > >        if (!vect_is_simple_use (vinfo, stmt_info, slp_node, 1,
> > >  			       &op1, &slp_op1, &dt[1], &vectype1_in)) […] @@
> > -12181,7
> > > +12235,6 @@ supportable_widening_operation (vec_info *vinfo,
> > >    return false;
> > >  }
> > >
> > > -
> > >  /* Function supportable_narrowing_operation
> > >
> > >     Check whether an operation represented by the code CODE is a
> > 
> > Seems like a spurious change.
> > 
> > > @@ -12205,7 +12258,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,
> > > +				 tree_code* _code1, int *multi_step_cvt,
> > 
> > The original formatting (space before the “*”) was correct.
> > Names beginning with _ are reserved, so I think we need a different
> > name here.  Also, the name in the comment should stay in sync with
> > the name in the code.
> > 
> > That said though, I'm not sure…
> > 
> > >                                   vec<tree> *interm_types)
> > >  {
> > >    machine_mode vec_mode;
> > > @@ -12217,8 +12270,8 @@ supportable_narrowing_operation (enum
> > tree_code code,
> > >    tree intermediate_type, prev_type;
> > >    machine_mode intermediate_mode, prev_mode;
> > >    int i;
> > > -  unsigned HOST_WIDE_INT n_elts;
> > >    bool uns;
> > > +  tree_code * code1 = (tree_code*) _code1;
> > 
> > …the combination of these two changes makes sense on their own.
> > 
> > >
> > >    *multi_step_cvt = 0;
> > >    switch (code)
> > > @@ -12227,9 +12280,8 @@ supportable_narrowing_operation (enum
> > tree_code code,
> > >        c1 = VEC_PACK_TRUNC_EXPR;
> > >        if (VECTOR_BOOLEAN_TYPE_P (narrow_vectype)
> > >  	  && VECTOR_BOOLEAN_TYPE_P (vectype)
> > > -	  && SCALAR_INT_MODE_P (TYPE_MODE (vectype))
> > > -	  && TYPE_VECTOR_SUBPARTS (vectype).is_constant (&n_elts)
> > > -	  && n_elts < BITS_PER_UNIT)
> > > +	  && TYPE_MODE (narrow_vectype) == TYPE_MODE (vectype)
> > > +	  && SCALAR_INT_MODE_P (TYPE_MODE (vectype)))
> > >  	optab1 = vec_pack_sbool_trunc_optab;
> > >        else
> > >  	optab1 = optab_for_tree_code (c1, vectype, optab_default);
> > > @@ -12320,9 +12372,8 @@ supportable_narrowing_operation (enum
> > tree_code code,
> > >  	  = lang_hooks.types.type_for_mode (intermediate_mode, uns);
> > >        if (VECTOR_BOOLEAN_TYPE_P (intermediate_type)
> > >  	  && VECTOR_BOOLEAN_TYPE_P (prev_type)
> > > -	  && SCALAR_INT_MODE_P (prev_mode)
> > > -	  && TYPE_VECTOR_SUBPARTS (intermediate_type).is_constant
> > (&n_elts)
> > > -	  && n_elts < BITS_PER_UNIT)
> > > +	  && intermediate_mode == prev_mode
> > > +	  && SCALAR_INT_MODE_P (prev_mode))
> > >  	interm_optab = vec_pack_sbool_trunc_optab;
> > >        else
> > >  	interm_optab
> > 
> > This part looks like a behavioural change, so I think it should be part
> > of a separate patch.
> > 
> > > diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
> > > index
> > 642eb0aeb21264cd736a479b1ec25357abef29cd..50ff8eeac1e6b9859302bd78
> > 4f10ee3d8ff4b4dc 100644
> > > --- a/gcc/tree-vectorizer.h
> > > +++ b/gcc/tree-vectorizer.h
> > > @@ -2120,13 +2120,12 @@ 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*, code_helper,
> > > +					    stmt_vec_info, tree, tree,
> > > +					    code_helper*, code_helper*,
> > > +					    int*, vec<tree> *);
> > >  extern bool supportable_narrowing_operation (enum tree_code, tree,
> > tree,
> > > -					     enum tree_code *, int *,
> > > +					     tree_code *, int *,
> > >  					     vec<tree> *);
> > >
> > >  extern unsigned record_stmt_cost (stmt_vector_for_cost *, int,
> > > diff --git a/gcc/tree.h b/gcc/tree.h
> > > index
> > f84958933d51144bb6ce7cc41eca5f7f06814550..00b0e4d1c696633fe38baad5
> > 295b1f90398d53fc 100644
> > > --- a/gcc/tree.h
> > > +++ b/gcc/tree.h
> > > @@ -92,6 +92,10 @@ public:
> > >    bool is_fn_code () const { return rep < 0; }
> > >    bool is_internal_fn () const;
> > >    bool is_builtin_fn () const;
> > > +  enum tree_code safe_as_tree_code () const { return is_tree_code () ?
> > > +    (tree_code)* this : MAX_TREE_CODES; }
> > > +  combined_fn safe_as_fn_code () const { return is_fn_code () ?
> > (combined_fn) *this
> > > +    : CFN_LAST;}
> > 
> > Since these don't fit on a line, the coding convention says that they
> > should be defined outside of the class.
> > 
> > Thanks,
> > Richard
> > 
> > >    int get_rep () const { return rep; }
> > >    bool operator== (const code_helper &other) { return rep == other.rep; }
> > >    bool operator!= (const code_helper &other) { return rep != other.rep; }
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)