From: Richard Sandiford <richard.sandiford@arm.com>
To: Joel Hutton <Joel.Hutton@arm.com>
Cc: Richard Biener <rguenther@suse.de>,
"gcc-patches\@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Subject: Re: [ping][vect-patterns] Refactor widen_plus/widen_minus as internal_fns
Date: Tue, 07 Jun 2022 09:18:20 +0100 [thread overview]
Message-ID: <mpt5ylckhtf.fsf@arm.com> (raw)
In-Reply-To: <DB9PR08MB6603F9C50FFDAF4B972EC317F5A29@DB9PR08MB6603.eurprd08.prod.outlook.com> (Joel Hutton's message of "Mon, 6 Jun 2022 18:20:02 +0100")
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..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; }
next prev parent reply other threads:[~2022-06-07 8:18 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-25 9:11 Joel Hutton
2022-05-27 13:23 ` Richard Biener
2022-05-31 10:07 ` Joel Hutton
2022-05-31 16:46 ` Tamar Christina
2022-06-01 10:11 ` Richard Biener
2022-06-06 17:20 ` Joel Hutton
2022-06-07 8:18 ` Richard Sandiford [this message]
2022-06-07 9:01 ` Joel Hutton
2022-06-09 14:03 ` Joel Hutton
2022-06-13 9:02 ` Richard Biener
2022-06-30 13:20 ` Joel Hutton
2022-07-12 12:32 ` Richard Biener
2023-03-17 10:14 ` Andre Vieira (lists)
2023-03-17 11:52 ` Richard Biener
2023-04-20 13:23 ` Andre Vieira (lists)
2023-04-24 11:57 ` Richard Biener
2023-04-24 13:01 ` Richard Sandiford
2023-04-25 12:30 ` Richard Biener
2023-04-28 16:06 ` Andre Vieira (lists)
2023-04-25 9:55 ` Andre Vieira (lists)
2023-04-28 12:36 ` [PATCH 1/3] Refactor to allow internal_fn's Andre Vieira (lists)
2023-05-03 11:55 ` Richard Biener
2023-05-04 15:20 ` Andre Vieira (lists)
2023-05-05 6:09 ` Richard Biener
2023-05-12 12:14 ` Andre Vieira (lists)
2023-05-12 13:18 ` Richard Biener
2023-04-28 12:37 ` [PATCH 2/3] Refactor widen_plus as internal_fn Andre Vieira (lists)
2023-05-03 12:11 ` Richard Biener
2023-05-03 19:07 ` Richard Sandiford
2023-05-12 12:16 ` Andre Vieira (lists)
2023-05-12 13:28 ` Richard Biener
2023-05-12 13:55 ` Andre Vieira (lists)
2023-05-12 14:01 ` Richard Sandiford
2023-05-15 10:20 ` Richard Biener
2023-05-15 10:47 ` Richard Sandiford
2023-05-15 11:01 ` Richard Biener
2023-05-15 11:10 ` Richard Sandiford
2023-05-15 11:53 ` Andre Vieira (lists)
2023-05-15 12:21 ` Richard Biener
2023-05-18 17:15 ` Andre Vieira (lists)
2023-05-22 13:06 ` Richard Biener
2023-06-01 16:27 ` Andre Vieira (lists)
2023-06-02 12:00 ` Richard Sandiford
2023-06-06 19:00 ` Jakub Jelinek
2023-06-06 21:28 ` [PATCH] modula2: Fix bootstrap Jakub Jelinek
2023-06-06 22:18 ` Gaius Mulley
2023-06-07 8:42 ` Andre Vieira (lists)
2023-06-13 14:48 ` Jakub Jelinek
2023-04-28 12:37 ` [PATCH 3/3] Remove widen_plus/minus_expr tree codes Andre Vieira (lists)
2023-05-03 12:29 ` Richard Biener
2023-05-10 9:15 ` Andre Vieira (lists)
2023-05-12 12:18 ` Andre Vieira (lists)
2022-06-13 9:18 ` [ping][vect-patterns] Refactor widen_plus/widen_minus as internal_fns Richard Biener
-- strict thread matches above, loose matches on Subject: below --
2021-11-25 10:08 Joel Hutton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=mpt5ylckhtf.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=Joel.Hutton@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=rguenther@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).