Re: [PATCH 2/3] Refactor widen_plus as internal_fn

[Richard Biener <rguenther@suse.de>]

On Fri, 28 Apr 2023, Andre Vieira (lists) wrote:

> This patch replaces the existing tree_code widen_plus and widen_minus
> patterns with internal_fn versions.
> 
> DEF_INTERNAL_OPTAB_HILO_FN is like DEF_INTERNAL_OPTAB_FN except it provides
> convenience wrappers for defining conversions that require a hi/lo split, like
> widening and narrowing operations.  Each definition for <NAME> will require an
> optab named <OPTAB> and two other optabs that you specify for signed and
> unsigned. The hi/lo pair is necessary because the widening 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.
> 
> DEF_INTERNAL_OPTAB_HILO_FN is used in internal-fn.def to register a widening
> internal_fn. It is defined differently in different places and internal-fn.def
> is sourced from those places so the parameters given can be reused.
>   internal-fn.c: defined to expand to hi/lo signed/unsigned optabs, later
> defined to generate the  'expand_' functions for the hi/lo versions of the fn.
>   internal-fn.def: defined to invoke DEF_INTERNAL_OPTAB_FN for the original
> and hi/lo variants of the internal_fn
> 
>  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>addl_hi_<mode> ->
> (u/s)addl2
>                        IFN_VEC_WIDEN_PLUS_LO  -> vec_widen_<su>addl_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.

I'll note that it's interesting we have widen multiplication as
the only existing example where we have both HI/LO and EVEN/ODD cases.
I think we want to share as much of the infrastructure to eventually
support targets doing even/odd (I guess all VLA vector targets will
be even/odd?).

DEF_INTERNAL_OPTAB_HILO_FN also looks to be implicitely directed to
widening operations (otherwise no signed/unsigned variants would be
necessary).  What I don't understand is why we need an optab
without _hi/_lo but in that case no signed/unsigned variant?

Looks like all plus, plus_lo and plus_hi are commutative but
only plus is widening?!  So is the setup that the vectorizer
doesn't know about the split and uses 'plus' but then the
expander performs the split?  It does look a bit awkward here
(the plain 'plus' is just used for the scalar case during
pattern recog it seems).

I'd rather have DEF_INTERNAL_OPTAB_HILO_FN split up, declaring
the hi/lo pairs and the scalar variant separately using
DEF_INTERNAL_FN without expander for that, and having
DEF_INTERNAL_HILO_WIDEN_OPTAB_FN and DEF_INTERNAL_EVENODD_WIDEN_OPTAB_FN
for the signed/unsigned pairs?  (if we need that helper at all)

Targets shouldn't need to implement the plain optab (it shouldn't
exist) and the vectorizer should query the hi/lo or even/odd
optabs for support instead.

The vectorizer parts look OK to me, I'd like Richard to chime
in on the optab parts as well.

Thanks,
Richard.

> gcc/ChangeLog:
> 
> 2023-04-28  Andre Vieira  <andre.simoesdiasvieira@arm.com>
>             Joel Hutton  <joel.hutton@arm.com>
>             Tamar Christina  <tamar.christina@arm.com>
> 
>     	* internal-fn.cc (INCLUDE_MAP): Include maps for use in optab
>     lookup.
>     	(DEF_INTERNAL_OPTAB_HILO_FN): Macro to define an internal_fn that
>     expands into multiple internal_fns (for widening).
> 	(ifn_cmp): Function to compare ifn's for sorting/searching.
> 	(lookup_hilo_ifn_optab): Add lookup function.
> 	(lookup_hilo_internal_fn): Add lookup function.
> 	(commutative_binary_fn_p): Add widen_plus fn's.
> 	(widening_fn_p): New function.
> 	(decomposes_to_hilo_fn_p): New function.
> 	* internal-fn.def (DEF_INTERNAL_OPTAB_HILO_FN): Define widening
>     plus,minus functions.
> 	(VEC_WIDEN_PLUS): Replacement for VEC_WIDEN_PLUS tree code.
> 	(VEC_WIDEN_MINUS): Replacement for VEC_WIDEN_MINUS tree code.
> 	* internal-fn.h (GCC_INTERNAL_FN_H): Add headers.
> 	(lookup_hilo_ifn_optab): Add prototype.
> 	(lookup_hilo_internal_fn): Likewise.
> 	(widening_fn_p): Likewise.
> 	(decomposes_to_hilo_fn_p): Likewise.
> 	* optabs.cc (commutative_optab_p): Add widening plus, minus optabs.
> 	* optabs.def (OPTAB_CD): widen add, sub optabs
> 	* tree-vect-patterns.cc (vect_recog_widen_op_pattern): Support
>     patterns with a hi/lo split.
>     	(vect_recog_widen_plus_pattern): Refactor to return
>     IFN_VECT_WIDEN_PLUS.
>     	(vect_recog_widen_minus_pattern): Refactor to return new
>     IFN_VEC_WIDEN_MINUS.
>     	* tree-vect-stmts.cc (vectorizable_conversion): Add widen plus/minus
>     ifn
>     support.
> 	(supportable_widening_operation): Add widen plus/minus ifn support.
> 
> 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)