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

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

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.

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.

>
> 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 *);
>>