From: Richard Biener <rguenther@suse.de>
To: Joel Hutton <Joel.Hutton@arm.com>
Cc: Richard Sandiford <Richard.Sandiford@arm.com>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Subject: Re: [ping][vect-patterns] Refactor widen_plus/widen_minus as internal_fns
Date: Fri, 27 May 2022 13:23:56 +0000 (UTC) [thread overview]
Message-ID: <nycvar.YFH.7.77.849.2205271323240.32424@jbgna.fhfr.qr> (raw)
In-Reply-To: <DB9PR08MB660348E126B0DBFBBC11325EF5D69@DB9PR08MB6603.eurprd08.prod.outlook.com>
On Wed, 25 May 2022, Joel Hutton wrote:
> Ping!
>
> Just checking there is still interest in this. I'm assuming you've been
> busy with release.
Can you post an updated patch (after the .cc renaming, and code_helper
now already moved to tree.h).
Thanks,
Richard.
> Joel
>
> > -----Original Message-----
> > From: Joel Hutton
> > Sent: 13 April 2022 16:53
> > To: Richard Sandiford <richard.sandiford@arm.com>
> > Cc: Richard Biener <rguenther@suse.de>; gcc-patches@gcc.gnu.org
> > Subject: [vect-patterns] Refactor widen_plus/widen_minus as internal_fns
> >
> > 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 */
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
next prev parent reply other threads:[~2022-05-27 13:23 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 [this message]
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
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=nycvar.YFH.7.77.849.2205271323240.32424@jbgna.fhfr.qr \
--to=rguenther@suse.de \
--cc=Joel.Hutton@arm.com \
--cc=Richard.Sandiford@arm.com \
--cc=gcc-patches@gcc.gnu.org \
/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).