RE: [PATCH 1/3]middle-end: Add the ability to let the target decide the method of argument promotions.

[Tamar Christina <Tamar.Christina@arm.com>]

> -----Original Message-----
> From: Richard Biener <rguenther@suse.de>
> Sent: Monday, May 16, 2022 7:31 AM
> To: Tamar Christina <Tamar.Christina@arm.com>
> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; jeffreyalaw@gmail.com;
> Richard Sandiford <Richard.Sandiford@arm.com>
> Subject: Re: [PATCH 1/3]middle-end: Add the ability to let the target decide
> the method of argument promotions.
> 
> On Fri, 13 May 2022, Tamar Christina wrote:
> 
> > Hi All,
> >
> > Some targets require function parameters to be promoted to a different
> > type on expand time because the target may not have native
> > instructions to work on such types.  As an example the AArch64 port
> > does not have native instructions working on integer 8- or 16-bit
> > values.  As such it promotes every parameter of these types to 32-bits.
> >
> > This promotion could be done by a target for two reasons:
> >
> > 1. For correctness.  This may be an APCS requirement for instance.
> > 2. For efficiency.  By promoting the argument at expansion time we don't
> have
> >    to keep promoting the type back and forth after each operation on it.
> >    i.e. the promotion simplies the RTL.
> >
> > This patch adds the ability for a target to decide whether during the
> > expansion to use an extend to handle promotion or to use a paradoxical
> subreg.
> >
> > A pradoxical subreg can be used when there's no correctness issues and
> > when you still want the RTL efficiency of not doing the promotion
> constantly.
> >
> > This also allows the target to not need to generate any code when the
> > top bits are not significant.
> >
> > An example is in AArch64 the following extend is unneeded:
> >
> > uint8_t fd2 (uint8_t xr){
> >     return xr + 1;
> > }
> >
> > currently generates:
> >
> > fd2:
> >         and     w0, w0, 255
> >         add     w0, w0, 1
> >         ret
> >
> > instead of
> >
> > fd2:
> >         add     w0, w0, #1
> >         ret
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> > Bootstrapped on x86_64-pc-linux-gnu and no issues
> >
> > Ok for master?
> 
> Why do we need a target hook for this?  Why doesn't the targets
> function_arg(?) hook just return (subreg:SI (reg:QI 0)) here instead when no
> zero-extension is required and (reg:QI 0) when it is?
> 

Because I'm not sure it's allowed to. From what I can tell function_arg expects
return registers to be hardregs. i.e. the actual APCS determined register for the
argument.  And since it's a hardreg can't make it a paradoxical subreg, it'll just
resize the register to the new mode.

assign_parm_setup_reg gets around this by creating a new pseudo and then
assigning the resulting rtl to the param set_parm_rtl (parm, rtl); which as far
as I can tell changes what expand will expand the parm to without actually
changing the actual APCS register.  Which is why I think the current code does
the promotions there.

> That said, an extra hook looks like a bad design to me, the existing
> cummulative args way of querying the target ABI shoud be enough, and if
> not, should be extended in a less hackish way.

I could perhaps modify function_arg_info to have a field for the subreg extension
which I can then use in function_arg for the promotion.  However I'll have a problem
with  the asserts in insert_value_copy_on_edge and set_rtl both of which check that
either the in/out types match, or the out type is a promoted in type.

set_rtl I can extend with enough information to determine whether the subreg was
intentional but insert_value_copy_on_edge doesn't give me access to enough information..

Which is probably why the current code re-queries the target..  Can I get to the parm information
Here?

I also initially tried to extend PROMOTE_MODE itself, however this is used by promote_mode, which
is used in many other places and I am not sure some of these usages are safe to change, such as
promote_ssa_mode.

The new hook I know only interacts with parms.  Any thoughts appreciated 😊

Cheers,
Tamar

> 
> But of course I am not familiar at all with the current state, but since you
> specifially CCed me ...
> 
> Richard.
> 
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> > 	* cfgexpand.cc (set_rtl): Check for function promotion.
> > 	* tree-outof-ssa.cc (insert_value_copy_on_edge): Likewise.
> > 	* function.cc (assign_parm_setup_reg): Likewise.
> > 	* hooks.cc (hook_bool_mode_mode_int_tree_false,
> > 	hook_bool_mode_mode_int_tree_true): New.
> > 	* hooks.h (hook_bool_mode_mode_int_tree_false,
> > 	hook_bool_mode_mode_int_tree_true): New.
> > 	* target.def (promote_function_args_subreg_p): New.
> > 	* doc/tm.texi: Document it.
> > 	* doc/tm.texi.in: Likewise.
> >
> > --- inline copy of patch --
> > diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc index
> >
> d3cc77d2ca98f620b29623fc5696410bad9bc184..df95184cfa185312c2a46cb92da
> a
> > 051718d9f4f3 100644
> > --- a/gcc/cfgexpand.cc
> > +++ b/gcc/cfgexpand.cc
> > @@ -206,14 +206,20 @@ set_rtl (tree t, rtx x)
> >       have to compute it ourselves.  For RESULT_DECLs, we accept mode
> >       mismatches too, as long as we have BLKmode or are not coalescing
> >       across variables, so that we don't reject BLKmode PARALLELs or
> > -     unpromoted REGs.  */
> > +     unpromoted REGs.  For any promoted types that result in a
> > +     paradoxical subreg also accept the argument.  */
> >    gcc_checking_assert (!x || x == pc_rtx || TREE_CODE (t) != SSA_NAME
> >  		       || (SSAVAR (t)
> >  			   && TREE_CODE (SSAVAR (t)) == RESULT_DECL
> >  			   && (promote_ssa_mode (t, NULL) == BLKmode
> >  			       || !flag_tree_coalesce_vars))
> >  		       || !use_register_for_decl (t)
> > -		       || GET_MODE (x) == promote_ssa_mode (t, NULL));
> > +		       || GET_MODE (x) == promote_ssa_mode (t, NULL)
> > +		       || targetm.calls.promote_function_args_subreg_p (
> > +					  GET_MODE (x),
> > +					  promote_ssa_mode (t, NULL),
> > +					  TYPE_UNSIGNED (TREE_TYPE (t)),
> > +					  SSAVAR (t)));
> >
> >    if (x)
> >      {
> > diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi index
> >
> 2f92d37da8c0091e9879a493cfe8a361eb1d9299..6314cd83a2488dc225d4a1a15
> 599
> > e8e51e639f7f 100644
> > --- a/gcc/doc/tm.texi
> > +++ b/gcc/doc/tm.texi
> > @@ -3906,6 +3906,15 @@ cases of mismatch, it also makes for better code
> on certain machines.
> >  The default is to not promote prototypes.
> >  @end deftypefn
> >
> > +@deftypefn {Target Hook} bool
> TARGET_PROMOTE_FUNCTION_ARGS_SUBREG_P
> > +(machine_mode @var{mode}, machine_mode @var{promoted_mode},
> int
> > +@var{unsignedp}, tree @var{v}) When a function argument is promoted
> > +with @code{PROMOTE_MODE} then this hook is used to determine
> whether
> > +the bits of the promoted type are all significant in the expression
> > +pointed to by V.  If they are an extend is generated, if they are not a
> paradoxical subreg is created for the argument from @code{mode} to
> @code{promoted_mode}.
> > +The default is to promote using an extend.
> > +@end deftypefn
> > +
> >  @deftypefn {Target Hook} bool TARGET_PUSH_ARGUMENT (unsigned int
> > @var{npush})  This target hook returns @code{true} if push
> > instructions will be  used to pass outgoing arguments.  When the push
> > instruction usage is diff --git a/gcc/doc/tm.texi.in
> > b/gcc/doc/tm.texi.in index
> >
> f869ddd5e5b8b7acbd8e9765fb103af24a1085b6..35f955803ec0a5a93be18a028
> fa1
> > 043f90858982 100644
> > --- a/gcc/doc/tm.texi.in
> > +++ b/gcc/doc/tm.texi.in
> > @@ -3103,6 +3103,8 @@ control passing certain arguments in registers.
> >
> >  @hook TARGET_PROMOTE_PROTOTYPES
> >
> > +@hook TARGET_PROMOTE_FUNCTION_ARGS_SUBREG_P
> > +
> >  @hook TARGET_PUSH_ARGUMENT
> >
> >  @defmac PUSH_ARGS_REVERSED
> > diff --git a/gcc/function.cc b/gcc/function.cc index
> >
> d5ed51a6a663a1ef472f5b1c090543f359c18f42..92f469bfd5d1ebfb09cc94d9be
> 85
> > 4715cd2f90f8 100644
> > --- a/gcc/function.cc
> > +++ b/gcc/function.cc
> > @@ -3161,7 +3161,7 @@ assign_parm_setup_reg (struct
> assign_parm_data_all *all, tree parm,
> >    machine_mode promoted_nominal_mode;
> >    int unsignedp = TYPE_UNSIGNED (TREE_TYPE (parm));
> >    bool did_conversion = false;
> > -  bool need_conversion, moved;
> > +  bool need_conversion, moved, use_subregs;
> >    enum insn_code icode;
> >    rtx rtl;
> >
> > @@ -3172,7 +3172,20 @@ assign_parm_setup_reg (struct
> assign_parm_data_all *all, tree parm,
> >      = promote_function_mode (data->nominal_type, data-
> >nominal_mode, &unsignedp,
> >  			     TREE_TYPE (current_function_decl), 2);
> >
> > -  parmreg = gen_reg_rtx (promoted_nominal_mode);
> > +  /* Check to see how the target wants the promotion of function
> arguments to
> > +     be handled.  */
> > +  use_subregs
> > +    = targetm.calls.promote_function_args_subreg_p (data-
> >nominal_mode,
> > +						    promoted_nominal_mode,
> > +						    unsignedp, parm);
> > +
> > +  /* If we're promoting using a paradoxical subreg then we need to keep
> using
> > +     the unpromoted type because that's the only fully defined value.
> > + */  if (use_subregs)
> > +    parmreg = gen_reg_rtx (data->nominal_mode);  else
> > +    parmreg = gen_reg_rtx (promoted_nominal_mode);
> > +
> >    if (!DECL_ARTIFICIAL (parm))
> >      mark_user_reg (parmreg);
> >
> > @@ -3256,9 +3269,19 @@ assign_parm_setup_reg (struct
> assign_parm_data_all *all, tree parm,
> >  	    }
> >  	  else
> >  	    t = op1;
> > -	  rtx_insn *pat = gen_extend_insn (op0, t,
> promoted_nominal_mode,
> > -					   data->passed_mode, unsignedp);
> > -	  emit_insn (pat);
> > +
> > +	  /* Promote the argument itself now if a target wants it.  This
> > +	     prevents unneeded back and forth convertions in RTL between
> > +	     the original and promoted type.  */
> > +	  if (use_subregs)
> > +	    emit_move_insn (op0, lowpart_subreg
> (promoted_nominal_mode, t,
> > +						 data->nominal_mode));
> > +	  else
> > +	    {
> > +	      rtx_insn *pat = gen_extend_insn (op0, t,
> promoted_nominal_mode,
> > +					       data->passed_mode, unsignedp);
> > +	      emit_insn (pat);
> > +	    }
> >  	  insns = get_insns ();
> >
> >  	  moved = true;
> > diff --git a/gcc/hooks.h b/gcc/hooks.h index
> >
> 1056e1e9e4dc3e6ce298557351047caa2f84227f..8d68de5cdb9adaea0a79ebf6d
> e59
> > 9f66b40aa67a 100644
> > --- a/gcc/hooks.h
> > +++ b/gcc/hooks.h
> > @@ -31,6 +31,8 @@ extern bool hook_bool_const_int_const_int_true
> > (const int, const int);  extern bool hook_bool_mode_false
> > (machine_mode);  extern bool hook_bool_mode_true (machine_mode);
> > extern bool hook_bool_mode_mode_true (machine_mode,
> machine_mode);
> > +extern bool hook_bool_mode_mode_int_tree_false (machine_mode,
> > +machine_mode, int, tree); extern bool
> > +hook_bool_mode_mode_int_tree_true (machine_mode,
> machine_mode, int,
> > +tree);
> >  extern bool hook_bool_mode_const_rtx_false (machine_mode,
> const_rtx);
> > extern bool hook_bool_mode_const_rtx_true (machine_mode,
> const_rtx);
> > extern bool hook_bool_mode_rtx_false (machine_mode, rtx); diff --git
> > a/gcc/hooks.cc b/gcc/hooks.cc index
> >
> b29233f4f852fb81ede75a5065d743cd16cc9219..7647774f9e8efbbe13d5607e4
> a4b
> > 2f1c9d22f045 100644
> > --- a/gcc/hooks.cc
> > +++ b/gcc/hooks.cc
> > @@ -89,6 +89,22 @@ hook_bool_mode_mode_true (machine_mode,
> machine_mode)
> >    return true;
> >  }
> >
> > +/* Generic hook that takes (machine_mode, machine_mode, int, tree)
> and
> > +   returns false.  */
> > +bool
> > +hook_bool_mode_mode_int_tree_false (machine_mode,
> machine_mode, int,
> > +tree) {
> > +  return false;
> > +}
> > +
> > +/* Generic hook that takes (machine_mode, machine_mode, int, tree)
> and
> > +   returns true.  */
> > +bool
> > +hook_bool_mode_mode_int_tree_true (machine_mode,
> machine_mode, int,
> > +tree) {
> > +  return true;
> > +}
> > +
> >  /* Generic hook that takes (machine_mode, const_rtx) and returns
> > false.  */  bool  hook_bool_mode_const_rtx_false (machine_mode,
> > const_rtx) diff --git a/gcc/target.def b/gcc/target.def index
> >
> 72c2e1ef756cf70a1c92abe81f8a6577eaaa2501..bdbacf8c5fd7b0626a37951f6f6
> e
> > c649f3194977 100644
> > --- a/gcc/target.def
> > +++ b/gcc/target.def
> > @@ -4561,6 +4561,17 @@ The default is to not promote prototypes.",
> >   bool, (const_tree fntype),
> >   hook_bool_const_tree_false)
> >
> > +DEFHOOK
> > +(promote_function_args_subreg_p,
> > + "When a function argument is promoted with @code{PROMOTE_MODE}
> then
> > +this\n\ hook is used to determine whether the bits of the promoted
> > +type are all\n\ significant in the expression pointed to by V.  If
> > +they are an extend is\n\ generated, if they are not a paradoxical
> > +subreg is created for the argument\n\ from @code{mode} to
> > +@code{promoted_mode}.\n\ The default is to promote using an
> extend.",
> > +bool, (machine_mode mode, machine_mode promoted_mode, int
> unsignedp,
> > +tree v),
> > + hook_bool_mode_mode_int_tree_false)
> > +
> >  DEFHOOK
> >  (struct_value_rtx,
> >   "This target hook should return the location of the structure
> > value\n\ diff --git a/gcc/tree-outof-ssa.cc b/gcc/tree-outof-ssa.cc
> > index
> >
> ec883126ad86d86a2c2dafee4592b8d83e2ed917..0f437023983baa0f23da25221
> f7b
> > ce8fc559a8b8 100644
> > --- a/gcc/tree-outof-ssa.cc
> > +++ b/gcc/tree-outof-ssa.cc
> > @@ -45,6 +45,7 @@ along with GCC; see the file COPYING3.  If not see
> > #include "tree-ssa-coalesce.h"
> >  #include "tree-outof-ssa.h"
> >  #include "dojump.h"
> > +#include "target.h"
> >
> >  /* FIXME: A lot of code here deals with expanding to RTL.  All that code
> >     should be in cfgexpand.cc.  */
> > @@ -333,7 +334,10 @@ insert_value_copy_on_edge (edge e, int dest,
> tree src, location_t locus)
> >    dest_mode = GET_MODE (dest_rtx);
> >    gcc_assert (src_mode == TYPE_MODE (TREE_TYPE (name)));
> >    gcc_assert (!REG_P (dest_rtx)
> > -	      || dest_mode == promote_ssa_mode (name, &unsignedp));
> > +	      || dest_mode == promote_ssa_mode (name, &unsignedp)
> > +	      || targetm.calls.promote_function_args_subreg_p (
> > +			dest_mode, promote_ssa_mode (name, NULL),
> unsignedp,
> > +			name));
> >
> >    if (src_mode != dest_mode)
> >      {
> >
> >
> >
> >
> >
> 
> --
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409
> Nuernberg, Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)