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

[Richard Biener <rguenther@suse.de>]

On Thu, 18 May 2023, Andre Vieira (lists) wrote:

> How about this?
> 
> Not sure about the DEF_INTERNAL documentation I rewrote in internal-fn.def,
> was struggling to word these, so improvements welcome!

The even/odd variant optabs are also commutative_optab_p, so is
the vec_widen_sadd without hi/lo or even/odd.

+/* { dg-options "-O3 -save-temps -fdump-tree-vect-all" } */

do you really want -all?  I think you want -details

+      else if (widening_fn_p (ifn)
+              || narrowing_fn_p (ifn))
+       {
+         tree lhs = gimple_get_lhs (stmt);
+         if (!lhs)
+           {
+             error ("vector IFN call with no lhs");
+             debug_generic_stmt (fn);

that's an error because ...?  Maybe we want to verify this
for all ECF_CONST|ECF_NOTHROW (or pure instead of const) internal
function calls, but I wouldn't add any verification as part
of this patch (not special to widening/narrowing fns either).

        if (gimple_call_internal_p (stmt))
-         return 0;
+         {
+           internal_fn fn = gimple_call_internal_fn (stmt);
+           switch (fn)
+             {
+             case IFN_VEC_WIDEN_PLUS_HI:
+             case IFN_VEC_WIDEN_PLUS_LO:
+             case IFN_VEC_WIDEN_MINUS_HI:
+             case IFN_VEC_WIDEN_MINUS_LO:
+               return 1;

this now looks incomplete.  I think that we want instead to
have a default: returning 1 and then special-cases we want
to cost as zero.  Not sure which - maybe blame tells why
this was added?  I think we can deal with this as followup
(likewise the ranger additions).

Otherwise looks good to me.

Thanks,
Richard.

> gcc/ChangeLog:
> 
> 2023-04-25  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 (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.
> 	         (direct_internal_fn_optab): Change visibility.
> 	* internal-fn.def (DEF_INTERNAL_WIDENING_OPTAB_FN): Macro to define an
>         internal_fn that expands into multiple internal_fns for widening.
>         (DEF_INTERNAL_NARROWING_OPTAB_FN): Likewise but for narrowing.
>         (IFN_VEC_WIDEN_PLUS, IFN_VEC_WIDEN_PLUS_HI, IFN_VEC_WIDEN_PLUS_LO,
>          IFN_VEC_WIDEN_PLUS_EVEN, IFN_VEC_WIDEN_PLUS_ODD,
>          IFN_VEC_WIDEN_MINUS, IFN_VEC_WIDEN_MINUS_HI, 
> IFN_VEC_WIDEN_MINUS_LO,
>          IFN_VEC_WIDEN_MINUS_ODD, IFN_VEC_WIDEN_MINUS_EVEN): Define widening
> 	         plus,minus functions.
> 	* internal-fn.h (direct_internal_fn_optab): Declare new prototype.
> 	(lookup_hilo_internal_fn): Likewise.
> 	(widening_fn_p): Likewise.
> 	(Narrowing_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 widening 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 or even/odd 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)