Joel Hutton <joel.hut...@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..50ff8eeac1e6b9859302bd784f10ee3d8ff4b4dc > 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..00b0e4d1c696633fe38baad5295b1f90398d53fc > 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; }