From: Richard Biener <rguenther@suse.de>
To: Richard Sandiford <richard.sandiford@arm.com>
Cc: "Andre Vieira (lists)" <andre.simoesdiasvieira@arm.com>,
Richard Biener <richard.guenther@gmail.com>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH 2/3] Refactor widen_plus as internal_fn
Date: Mon, 15 May 2023 10:20:54 +0000 (UTC) [thread overview]
Message-ID: <nycvar.YFH.7.77.849.2305151018120.4723@jbgna.fhfr.qr> (raw)
In-Reply-To: <mptleht8x9b.fsf@arm.com>
On Fri, 12 May 2023, Richard Sandiford wrote:
> Richard Biener <rguenther@suse.de> writes:
> > On Fri, 12 May 2023, Andre Vieira (lists) wrote:
> >
> >> I have dealt with, I think..., most of your comments. There's quite a few
> >> changes, I think it's all a bit simpler now. I made some other changes to the
> >> costing in tree-inline.cc and gimple-range-op.cc in which I try to preserve
> >> the same behaviour as we had with the tree codes before. Also added some extra
> >> checks to tree-cfg.cc that made sense to me.
> >>
> >> I am still regression testing the gimple-range-op change, as that was a last
> >> minute change, but the rest survived a bootstrap and regression test on
> >> aarch64-unknown-linux-gnu.
> >>
> >> cover letter:
> >>
> >> This patch replaces the existing tree_code widen_plus and widen_minus
> >> patterns with internal_fn versions.
> >>
> >> DEF_INTERNAL_OPTAB_WIDENING_HILO_FN and DEF_INTERNAL_OPTAB_NARROWING_HILO_FN
> >> are like DEF_INTERNAL_SIGNED_OPTAB_FN and DEF_INTERNAL_OPTAB_FN respectively
> >> except they provide convenience wrappers for defining conversions that require
> >> a hi/lo split. Each definition for <NAME> will require optabs for _hi and _lo
> >> and each of those will also require a signed and unsigned version in the case
> >> of widening. The hi/lo pair is necessary because the widening and narrowing
> >> operations take n narrow elements as inputs and return n/2 wide elements as
> >> outputs. The 'lo' operation operates on the first n/2 elements of input. The
> >> 'hi' operation operates on the second n/2 elements of input. Defining an
> >> internal_fn along with hi/lo variations allows a single internal function to
> >> be returned from a vect_recog function that will later be expanded to hi/lo.
> >>
> >>
> >> For example:
> >> IFN_VEC_WIDEN_PLUS -> IFN_VEC_WIDEN_PLUS_HI, IFN_VEC_WIDEN_PLUS_LO
> >> for aarch64: IFN_VEC_WIDEN_PLUS_HI -> vec_widen_<su>add_hi_<mode> ->
> >> (u/s)addl2
> >> IFN_VEC_WIDEN_PLUS_LO -> vec_widen_<su>add_lo_<mode>
> >> -> (u/s)addl
> >>
> >> This gives the same functionality as the previous WIDEN_PLUS/WIDEN_MINUS tree
> >> codes which are expanded into VEC_WIDEN_PLUS_LO, VEC_WIDEN_PLUS_HI.
> >
> > What I still don't understand is how we are so narrowly focused on
> > HI/LO? We need a combined scalar IFN for pattern selection (not
> > sure why that's now called _HILO, I expected no suffix). Then there's
> > three possibilities the target can implement this:
> >
> > 1) with a widen_[su]add<mode> instruction - I _think_ that's what
> > RISCV is going to offer since it is a target where vector modes
> > have "padding" (aka you cannot subreg a V2SI to get V4HI). Instead
> > RVV can do a V4HI to V4SI widening and widening add/subtract
> > using vwadd[u] and vwsub[u] (the HI->SI widening is actually
> > done with a widening add of zero - eh).
> > IIRC GCN is the same here.
>
> SVE currently does this too, but the addition and widening are
> separate operations. E.g. in principle there's no reason why
> you can't sign-extend one operand, zero-extend the other, and
> then add the result together. Or you could extend them from
> different sizes (QI and HI). All of those are supported
> (if the costing allows them).
I see. So why does the target the expose widen_[su]add<mode> at all?
> If the target has operations to do combined extending and adding (or
> whatever), then at the moment we rely on combine to generate them.
>
> So I think this case is separate from Andre's work. The addition
> itself is just an ordinary addition, and any widening happens by
> vectorising a CONVERT/NOP_EXPR.
>
> > 2) with a widen_[su]add{_lo,_hi}<mode> combo - that's what the tree
> > codes currently support (exclusively)
> > 3) similar, but widen_[su]add{_even,_odd}<mode>
> >
> > that said, things like decomposes_to_hilo_fn_p look to paint us into
> > a 2) corner without good reason.
>
> I suppose one question is: how much of the patch is really specific
> to HI/LO, and how much is just grouping two halves together?
Yep, that I don't know for sure.
> The nice
> thing about the internal-fn grouping macros is that, if (3) is
> implemented in future, the structure will strongly encourage even/odd
> pairs to be supported for all operations that support hi/lo. That is,
> I would expect the grouping macros to be extended to define even/odd
> ifns alongside hi/lo ones, rather than adding separate definitions
> for even/odd functions.
>
> If so, at least from the internal-fn.* side of things, I think the question
> is whether it's OK to stick with hilo names for now, or whether we should
> use more forward-looking names.
I think for parts that are independent we could use a more
forward-looking name. Maybe _halves? But I'm also not sure
how much of that is really needed (it seems to be tied around
optimizing optabs space?)
Richard.
> Thanks,
> Richard
>
> >
> > Richard.
> >
> >> gcc/ChangeLog:
> >>
> >> 2023-05-12 Andre Vieira <andre.simoesdiasvieira@arm.com>
> >> Joel Hutton <joel.hutton@arm.com>
> >> Tamar Christina <tamar.christina@arm.com>
> >>
> >> * config/aarch64/aarch64-simd.md (vec_widen_<su>addl_lo_<mode>):
> >> Rename
> >> this ...
> >> (vec_widen_<su>add_lo_<mode>): ... to this.
> >> (vec_widen_<su>addl_hi_<mode>): Rename this ...
> >> (vec_widen_<su>add_hi_<mode>): ... to this.
> >> (vec_widen_<su>subl_lo_<mode>): Rename this ...
> >> (vec_widen_<su>sub_lo_<mode>): ... to this.
> >> (vec_widen_<su>subl_hi_<mode>): Rename this ...
> >> (vec_widen_<su>sub_hi_<mode>): ...to this.
> >> * doc/generic.texi: Document new IFN codes.
> >> * internal-fn.cc (DEF_INTERNAL_OPTAB_WIDENING_HILO_FN): Macro to
> >> define an
> >> internal_fn that expands into multiple internal_fns for widening.
> >> (DEF_INTERNAL_OPTAB_NARROWING_HILO_FN): Likewise but for narrowing.
> >> (ifn_cmp): Function to compare ifn's for sorting/searching.
> >> (lookup_hilo_internal_fn): Add lookup function.
> >> (commutative_binary_fn_p): Add widen_plus fn's.
> >> (widening_fn_p): New function.
> >> (narrowing_fn_p): New function.
> >> (decomposes_to_hilo_fn_p): New function.
> >> (direct_internal_fn_optab): Change visibility.
> >> * internal-fn.def (DEF_INTERNAL_OPTAB_WIDENING_HILO_FN): Define
> >> widening
> >> plus,minus functions.
> >> (VEC_WIDEN_PLUS): Replacement for VEC_WIDEN_PLUS_EXPR tree code.
> >> (VEC_WIDEN_MINUS): Replacement for VEC_WIDEN_MINUS_EXPR tree code.
> >> * internal-fn.h (GCC_INTERNAL_FN_H): Add headers.
> >> (direct_internal_fn_optab): Declare new prototype.
> >> (lookup_hilo_internal_fn): Likewise.
> >> (widening_fn_p): Likewise.
> >> (Narrowing_fn_p): Likewise.
> >> (decomposes_to_hilo_fn_p): Likewise.
> >> * optabs.cc (commutative_optab_p): Add widening plus optabs.
> >> * optabs.def (OPTAB_D): Define widen add, sub optabs.
> >> * tree-cfg.cc (verify_gimple_call): Add checks for new widen
> >> add and sub IFNs.
> >> * tree-inline.cc (estimate_num_insns): Return same
> >> cost for widen add and sub IFNs as previous tree_codes.
> >> * tree-vect-patterns.cc (vect_recog_widen_op_pattern): Support
> >> patterns with a hi/lo split.
> >> (vect_recog_sad_pattern): Refactor to use new IFN codes.
> >> (vect_recog_widen_plus_pattern): Likewise.
> >> (vect_recog_widen_minus_pattern): Likewise.
> >> (vect_recog_average_pattern): Likewise.
> >> * tree-vect-stmts.cc (vectorizable_conversion): Add support for
> >> _HILO IFNs.
> >> (supportable_widening_operation): Likewise.
> >> * tree.def (WIDEN_SUM_EXPR): Update example to use new IFNs.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >> * gcc.target/aarch64/vect-widen-add.c: Test that new
> >> IFN_VEC_WIDEN_PLUS is being used.
> >> * gcc.target/aarch64/vect-widen-sub.c: Test that new
> >> IFN_VEC_WIDEN_MINUS is being used.
> >>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
next prev parent reply other threads:[~2023-05-15 10:20 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-25 9:11 [ping][vect-patterns] Refactor widen_plus/widen_minus as internal_fns 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
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 [this message]
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
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.2305151018120.4723@jbgna.fhfr.qr \
--to=rguenther@suse.de \
--cc=andre.simoesdiasvieira@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=richard.guenther@gmail.com \
--cc=richard.sandiford@arm.com \
/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).