> > 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.
>
>
> - if (TREE_CODE (gimple_assign_lhs (stmt)) != SSA_NAME)
> + if (gimple_get_lhs (stmt) == NULL_TREE ||
> + TREE_CODE(gimple_get_lhs (stmt)) != SSA_NAME)
> return false;
>
> || go to the next line, space after TREE_CODE
>
Done.
> + bool widen_arith = false;
> + gimple_match_op res_op;
> + if (!gimple_extract_op (stmt, &res_op))
> + return false;
> + code = res_op.code;
> + op_type = res_op.num_ops;
> +
> + if (is_gimple_assign (stmt))
> + {
> + widen_arith = (code == WIDEN_PLUS_EXPR
> + || code == WIDEN_MINUS_EXPR
> + || code == WIDEN_MULT_EXPR
> + || code == WIDEN_LSHIFT_EXPR);
> + }
> + else
> + widen_arith = gimple_call_flags (stmt) & ECF_WIDEN;
>
> there seem to be formatting issues. Also shouldn't you check
> if (res_op.code.is_tree_code ()) instead if is_gimple_assign?
> I also don't like the ECF_WIDEN "trick", just do as with the
> tree codes and explicitely enumerate widening ifns here.
>
Done. I've set widen_arith to False for the first patch as the second patch
introduces the widening ifns.
> gimple_extract_op is a bit heavy-weight as well, so maybe
> instead simply do
>
> if (is_gimple_assign (stmt))
> {
> code = gimple_assign_rhs_code (stmt);
> ...
> }
> else if (gimple_call_internal_p (stmt))
> {
> code = gimple_call_internal_fn (stmt);
> ...
> }
> else
> return false;
The patch was originally written as above, it was changed to use
gimple_extract_op at the request of Richard Sandiford. I prefer
gimple_extract_op as it's more compact, but I don't have strong feelings. If
the Richards can agree on either version I'm happy.
>From Richard Sandiford:
> > + 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.
>
> + code_helper c1=MAX_TREE_CODES, c2=MAX_TREE_CODES;
>
> spaces before/after '='
>
Done.
> @@ -12061,13 +12105,16 @@ 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)
> {
>
> unnecessary whitespace change.
>
Fixed.
> diff --git a/gcc/tree.h b/gcc/tree.h
> index
> f84958933d51144bb6ce7cc41eca5f7f06814550..e51e34c051d9b91d1c02a4b2
> fefdb2b15606a36f
> 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 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;}
>
> hmm, the other as_* functions we have are not member functions.
> Also this semantically differs from the tree_code () conversion
> operator (that one was supposed to be "cheap"). The existing
> as_internal_fn for example is documented as being valid only if
> the code is actually an internal fn. I see you are introducing
> the new function as convenience to get a "safe" not-a-X value,
> so maybe they should be called safe_as_tree_code () instead?
>
SGTM. Done
>
> 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; }
> @@ -6657,6 +6661,54 @@ 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
> +{
>
> duplicate add of code_helper.
Fixed.
Tests are being re-run.
Ok, with changes?
0001-Refactor-to-allow-internal_fn-s.patch
Description: 0001-Refactor-to-allow-internal_fn-s.patch
0002-Refactor-widen_plus-as-internal_fn.patch
Description: 0002-Refactor-widen_plus-as-internal_fn.patch
0003-Remove-widen_plus-minus_expr-tree-codes.patch
Description: 0003-Remove-widen_plus-minus_expr-tree-codes.patch
