Hi!
Thanks so much for your review!
Richard Biener <rguent...@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
>>
>> @@ -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
>> *);
>>
