Re: [PATCH] Using sub-scalars mode to move struct block

[Jiufu Guo <guojiufu@linux.ibm.com>]

Hi!

Thanks for your helpful comments/sugguestions!

Richard Biener <rguenther@suse.de> writes:

> On Mon, 14 Nov 2022, Jiufu Guo wrote:
>
>> 
>> Hi!
>> Thanks so much for your review!
>> 
>> Richard Biener <rguenther@suse.de> writes:
>> 
>> > On Fri, 11 Nov 2022, Jiufu Guo wrote:
>> >
>> >> Hi,
>> >> 
>> >> When assigning a struct parameter to another variable, or loading a
>> >> memory block to a struct var (especially for return value),
>> >> Now, "block move" would be used during expand the assignment. And
>> >> the "block move" may use a type/mode different from the mode which
>> >> is accessing the var. e.g. on ppc64le, V2DI would be used to move
>> >> the block of 16bytes.
>> >> 
>> >> And then, this "block move" would prevent optimization passes from
>> >> leaping/crossing over the assignment. PR65421 reflects this issue.
>> >> 
>> >> As the example code in PR65421.
>> >> 
>> >> typedef struct { double a[4]; } A;
>> >> A foo (const A *a) { return *a; }
>> >> 
>> >> On ppc64le, the below instructions are used for the "block move":
>> >>   7: r122:V2DI=[r121:DI]
>> >>   8: r124:V2DI=[r121:DI+r123:DI]
>> >>   9: [r112:DI]=r122:V2DI
>> >>   10: [r112:DI+0x10]=r124:V2DI
>> >> 
>> >> For this issue, a few comments/suggestions are mentioned via RFC:
>> >> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/604646.html
>> >> I drafted a patch which is updating the behavior of block_move for
>> >> struct type. This patch is simple to work with, a few ideas in the
>> >> comments are not put into this patch. I would submit this
>> >> patch first.
>> >> 
>> >> The idea is trying to use sub-modes(scalar) for the "block move".
>> >> And the sub-modes would align with the access patterns of the
>> >> struct members and usages on parameter/return value.
>> >> The major benefits of this change would be raising more 
>> >> opportunities for other optimization passes(cse/dse/xprop).
>> >> 
>> >> The suitable mode would be target specified and relates to ABI,
>> >> this patch introduces a target hook. And in this patch, the hook
>> >> is implemented on rs6000.
>> >> 
>> >> In this patch, the hook would be just using heuristic modes for all
>> >> struct block moving. And the hook would not check if the "block move"
>> >> is about parameters or return value or other uses.
>> >> 
>> >> For the rs6000 implementation of this hook, it is able to use
>> >> DF/DI/TD/.. modes for the struct block movement. The sub-modes
>> >> would be the same as the mode when the struct type is on parameter or
>> >> return value.
>> >> 
>> >> Bootstrapped and regtested on ppc64/ppc64le. 
>> >> Is this ok for trunk?
>> >> 
>> >> 
>> >> BR,
>> >> Jeff(Jiufu)
>> >> 
>> >> 
>> >> gcc/ChangeLog:
>> >> 
>> >> 	* config/rs6000/rs6000.cc (TARGET_BLOCK_MOVE_FOR_STRUCT): Define.
>> >> 	(submode_for_struct_block_move): New function.  Called from
>> >> 	rs600_block_move_for_struct.
>> >> 	(rs600_block_move_for_struct): New function.
>> >> 	* doc/tm.texi: Regenerate.
>> >> 	* doc/tm.texi.in (TARGET_BLOCK_MOVE_FOR_STRUCT): New.
>> >> 	* expr.cc (store_expr): Call block_move_for_struct.
>> >> 	* target.def (block_move_for_struct): New hook.
>> >> 	* targhooks.cc (default_block_move_for_struct): New function.
>> >> 	* targhooks.h (default_block_move_for_struct): New Prototype.
>> >> 
>> >> ---
>> >>  gcc/config/rs6000/rs6000.cc | 44 +++++++++++++++++++++++++++++++++++++
>> >>  gcc/doc/tm.texi             |  6 +++++
>> >>  gcc/doc/tm.texi.in          |  2 ++
>> >>  gcc/expr.cc                 | 14 +++++++++---
>> >>  gcc/target.def              | 10 +++++++++
>> >>  gcc/targhooks.cc            |  7 ++++++
>> >>  gcc/targhooks.h             |  1 +
>> >>  7 files changed, 81 insertions(+), 3 deletions(-)
>> >> 
>> >> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
>> >> index a85d7630b41..e14cecba0ef 100644
>> >> --- a/gcc/config/rs6000/rs6000.cc
>> >> +++ b/gcc/config/rs6000/rs6000.cc
>> >> @@ -1758,6 +1758,9 @@ static const struct attribute_spec rs6000_attribute_table[] =
>> >>  #undef TARGET_NEED_IPA_FN_TARGET_INFO
>> >>  #define TARGET_NEED_IPA_FN_TARGET_INFO rs6000_need_ipa_fn_target_info
>> >>  
>> >> +#undef TARGET_BLOCK_MOVE_FOR_STRUCT
>> >> +#define TARGET_BLOCK_MOVE_FOR_STRUCT rs600_block_move_for_struct
>> >> +
>> >>  #undef TARGET_UPDATE_IPA_FN_TARGET_INFO
>> >>  #define TARGET_UPDATE_IPA_FN_TARGET_INFO rs6000_update_ipa_fn_target_info
>> >>  \f
>> >> @@ -23672,6 +23675,47 @@ rs6000_function_value (const_tree valtype,
>> >>    return gen_rtx_REG (mode, regno);
>> >>  }
>> >>  
>> >> +/* Subroutine of rs600_block_move_for_struct, to get the internal mode which
>> >> +   would be used to move the struct.  */
>> >> +static machine_mode
>> >> +submode_for_struct_block_move (tree type)
>> >> +{
>> >> +  gcc_assert (TREE_CODE (type) == RECORD_TYPE);
>> >> +
>> >> +  /* The sub mode may not be the field's type of the struct.
>> >> +     It would be fine to use the mode as if the type is used as a function
>> >> +     parameter or return value.  For example: DF for "{double a[4];}", and
>> >> +     DI for "{doubel a[3]; long l;}".
>> >> +     Here, using the mode as if it is function return type.  */
>> >> +  rtx val = rs6000_function_value (type, NULL, 0);
>> >> +  return (GET_CODE (val) == PARALLEL) ? GET_MODE (XEXP (XVECEXP (val, 0, 0), 0))
>> >> +				      : word_mode;
>> >> +}
>> >> +
>> >> +/* Implement the TARGET_BLOCK_MOVE_FOR_STRUCT hook.  */
>> >> +static void
>> >> +rs600_block_move_for_struct (rtx x, rtx y, tree exp, HOST_WIDE_INT method)
>> >> +{
>> >> +  machine_mode mode = submode_for_struct_block_move (TREE_TYPE (exp));
>> >> +  int mode_size = GET_MODE_SIZE (mode);
>> >> +  int size = UINTVAL (expr_size (exp));
>> >> +  if (size < mode_size || (size % mode_size) != 0 || size > 64)
>> >> +    {
>> >> +      default_block_move_for_struct (x, y, exp, method);
>> >> +      return;
>> >> +    }
>> >> +
>> >> +  int len = size / mode_size;
>> >> +  for (int i = 0; i < len; i++)
>> >> +    {
>> >> +      rtx temp = gen_reg_rtx (mode);
>> >> +      rtx src = adjust_address (y, mode, mode_size * i);
>> >> +      rtx dest = adjust_address (x, mode, mode_size * i);
>> >> +      emit_move_insn (temp, src);
>> >> +      emit_move_insn (dest, temp);
>> >> +    }
>> >> +}
>> >> +
>> >>  /* Define how to find the value returned by a library function
>> >>     assuming the value has mode MODE.  */
>> >>  rtx
>> >> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
>> >> index 8572313b308..c8a1c1f30cf 100644
>> >> --- a/gcc/doc/tm.texi
>> >> +++ b/gcc/doc/tm.texi
>> >> @@ -1380,6 +1380,12 @@ retain the field's mode.
>> >>  Normally, this is not needed.
>> >>  @end deftypefn
>> >>  
>> >> +@deftypefn {Target Hook} void TARGET_BLOCK_MOVE_FOR_STRUCT (rtx @var{x}, rtx @var{y}, tree @var{exp}, HOST_WIDE_INT @var{method})
>> >> +Move from @var{y} to @var{x}, where @var{y} is as @var{exp} in structure
>> >> +type. @var{method} is the method if this function invokes block_move.
>> >> +The default definition invokes block_move.
>> >> +@end deftypefn
>> >> +
>> >>  @defmac ROUND_TYPE_ALIGN (@var{type}, @var{computed}, @var{specified})
>> >>  Define this macro as an expression for the alignment of a type (given
>> >>  by @var{type} as a tree node) if the alignment computed in the usual
>> >> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
>> >> index 986e8f0da09..f0f525a2008 100644
>> >> --- a/gcc/doc/tm.texi.in
>> >> +++ b/gcc/doc/tm.texi.in
>> >> @@ -1226,6 +1226,8 @@ to aligning a bit-field within the structure.
>> >>  
>> >>  @hook TARGET_MEMBER_TYPE_FORCES_BLK
>> >>  
>> >> +@hook TARGET_BLOCK_MOVE_FOR_STRUCT
>> >> +
>> >>  @defmac ROUND_TYPE_ALIGN (@var{type}, @var{computed}, @var{specified})
>> >>  Define this macro as an expression for the alignment of a type (given
>> >>  by @var{type} as a tree node) if the alignment computed in the usual
>> >> diff --git a/gcc/expr.cc b/gcc/expr.cc
>> >> index c6917fbf7bd..734dc07a76b 100644
>> >> --- a/gcc/expr.cc
>> >> +++ b/gcc/expr.cc
>> >> @@ -6504,9 +6504,17 @@ store_expr (tree exp, rtx target, int call_param_p,
>> >>  	emit_group_store (target, temp, TREE_TYPE (exp),
>> >>  			  int_size_in_bytes (TREE_TYPE (exp)));
>> >>        else if (GET_MODE (temp) == BLKmode)
>> >> -	emit_block_move (target, temp, expr_size (exp),
>> >> -			 (call_param_p
>> >> -			  ? BLOCK_OP_CALL_PARM : BLOCK_OP_NORMAL));
>> >> +	{
>> >> +	  if (TREE_CODE (TREE_TYPE (exp)) == RECORD_TYPE)
>> >> +	    targetm.block_move_for_struct (target, temp, exp,
>> >> +					   call_param_p ? BLOCK_OP_CALL_PARM
>> >> +							: BLOCK_OP_NORMAL);
>> >
>> > I think it's only sensible to do something about the call_param_p case,
>> > all other cases are GIGO.  And I don't see a good reason to add a new
>> > target hook since function parameter passing info is readily
>> > available.
>> This patch enhances the normal assignments on struct variables (
>> expand_assignment-->store_expr, where the call_param_p is 0). Because
>> the "struct block move/copy" is generated for this case too.
>> 
>> For example, the struct block_move is generated.
>> # .MEM_3 = VDEF <.MEM_1(D)>
>> D.3956 = *a_2(D);
>> For this stmt, "D.3956" is a struct variable, and "a_2(D)" points to a
>> block of the struct type.
>> 
>> If we only care about parameter/arg/retval(s), we would not need a
>> hook, as you said, since parm/retval contains the information about
>> the target/ABI.  And for more as you suggested in the previous emails,
>> we could do SRA-like analyze to check if an assignment relates to
>> parm/retval.
>
> But the issue with applying a stmt-local heuristic is that it's going
> to be as many times wrong as it is right so there inevitably will be
> regressions.
Understant. :-)  In the patch, the heurisitic is aplied to all
assignment on struct (no matter parm/return related).  The generated
binary would be changed definitly: mem move(via vector loads/stores)
for struct type becomes through sub-modes scalar loads/stores.
The binary change may cause some difference(e.g. performance change).
Even I had spec2017 tests on some machines(P9 and P10), it is sitll
possible has regressions.

>
> Also copies not directly involving parameters or returns can be optimized
> on the GIMPLE level just fine (see what SRA does), the exception is
> cpymem optab implementations but those you short-circuit as well here.
Yes, agree. So, enhancing the stmt/assignment about param/retval would
fix the issue (with low regression risks). 

>
>> The current patch is simple relatively, it uses 'scalar-modes' to move
>> blocks for all struct-type assignments.  I feel this would be enough,
>> and I'm wondering if we need to check parm/retval.
>
> It's only for the param/retval that RTL expansion has more information
> than GIMPLE and in that specific case the code generation is very 
> difficult to untie and thus a change like this looks appropriate.
Understant, I will update patch to handle more about param/retval.

Thanks again!

BR,
Jeff (Jiufu)

>
> Thanks,
> Richard.
>
>> 
>> >
>> > The question is whether this is best handled up in the call chain where
>> > the parameter setup is done or below here (or in emit_block_move).
>> As above discussed, this patch touches 'block move' for struct type,
>> including copy from 'parameter stack block' to a variable block.
>> Oh, assigning from 'parameter regs' to 'stack block' is not changed
>> here, it is done through "assign_parms-...->emit_group_xx".
>> 
>> If any misunderstanding, or any comments/concerns, please point them
>> out.  Thanks a lot!
>> 
>> BR,
>> Jeff (Jiufu)
>> 
>> >
>> > Richard.
>> >
>> >> +	  else
>> >> +	    emit_block_move (target, temp, expr_size (exp),
>> >> +			     (call_param_p ? BLOCK_OP_CALL_PARM
>> >> +					   : BLOCK_OP_NORMAL));
>> >> +	}
>> >> +
>> >>        /* If we emit a nontemporal store, there is nothing else to do.  */
>> >>        else if (nontemporal && emit_storent_insn (target, temp))
>> >>  	;
>> >> diff --git a/gcc/target.def b/gcc/target.def
>> >> index 25f94c19fa7..e141f72a8a3 100644
>> >> --- a/gcc/target.def
>> >> +++ b/gcc/target.def
>> >> @@ -5584,6 +5584,16 @@ Normally, this is not needed.",
>> >>   bool, (const_tree field, machine_mode mode),
>> >>   default_member_type_forces_blk)
>> >>  
>> >> +
>> >> +/* Move block for structure type.  */
>> >> +DEFHOOK
>> >> +(block_move_for_struct,
>> >> + "Move from @var{y} to @var{x}, where @var{y} is as @var{exp} in structure\n\
>> >> +type. @var{method} is the method if this function invokes block_move.\n\
>> >> +The default definition invokes block_move.",
>> >> + void, (rtx x, rtx y, tree exp, HOST_WIDE_INT method),
>> >> + default_block_move_for_struct)
>> >> +
>> >>  /* See tree-ssa-math-opts.cc:divmod_candidate_p for conditions
>> >>     that gate the divod transform.  */
>> >>  DEFHOOK
>> >> diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc
>> >> index 12a58456b39..2b96c348419 100644
>> >> --- a/gcc/targhooks.cc
>> >> +++ b/gcc/targhooks.cc
>> >> @@ -2330,6 +2330,13 @@ default_member_type_forces_blk (const_tree, machine_mode)
>> >>    return false;
>> >>  }
>> >>  
>> >> +/* Default version of block_move_for_struct.  */
>> >> +void
>> >> +default_block_move_for_struct (rtx x, rtx y, tree exp, HOST_WIDE_INT method)
>> >> +{
>> >> +  emit_block_move (x, y, expr_size (exp), (enum block_op_methods)method);
>> >> +}
>> >> +
>> >>  /* Default version of canonicalize_comparison.  */
>> >>  
>> >>  void
>> >> diff --git a/gcc/targhooks.h b/gcc/targhooks.h
>> >> index a6a423c1abb..c284a35ee28 100644
>> >> --- a/gcc/targhooks.h
>> >> +++ b/gcc/targhooks.h
>> >> @@ -264,6 +264,7 @@ extern void default_asm_output_ident_directive (const char*);
>> >>  
>> >>  extern scalar_int_mode default_cstore_mode (enum insn_code);
>> >>  extern bool default_member_type_forces_blk (const_tree, machine_mode);
>> >> +extern void default_block_move_for_struct (rtx, rtx, tree, HOST_WIDE_INT);
>> >>  extern void default_atomic_assign_expand_fenv (tree *, tree *, tree *);
>> >>  extern tree build_va_arg_indirect_ref (tree);
>> >>  extern tree std_gimplify_va_arg_expr (tree, tree, gimple_seq *, gimple_seq *);
>> >> 
>>