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

[Richard Biener <rguenther@suse.de>]

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?

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.

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..df95184cfa185312c2a46cb92daa051718d9f4f3 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..6314cd83a2488dc225d4a1a15599e8e51e639f7f 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..35f955803ec0a5a93be18a028fa1043f90858982 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..92f469bfd5d1ebfb09cc94d9be854715cd2f90f8 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..8d68de5cdb9adaea0a79ebf6de599f66b40aa67a 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..7647774f9e8efbbe13d5607e4a4b2f1c9d22f045 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..bdbacf8c5fd7b0626a37951f6f6ec649f3194977 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..0f437023983baa0f23da25221f7bce8fc559a8b8 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)