Re: [PATCH V2] Use subscalar mode to move struct block for parameter

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

Hi Jeff,

Thanks a lot for your comments!

Jeff Law <jeffreyalaw@gmail.com> writes:

> On 11/20/22 20:07, Jiufu Guo wrote:
>> Jiufu Guo <guojiufu@linux.ibm.com> writes:
>>
>>> Hi,
>>>
>>> As mentioned in the previous version patch:
>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/604646.html
>>> The suboptimal code is generated for "assigning from parameter" or
>>> "assigning to return value".
>>> This patch enhances the assignment from parameters like the below
>>> cases:
>>> /////case1.c
>>> typedef struct SA {double a[3];long l; } A;
>>> A ret_arg (A a) {return a;}
>>> void st_arg (A a, A *p) {*p = a;}
>>>
>>> ////case2.c
>>> typedef struct SA {double a[3];} A;
>>> A ret_arg (A a) {return a;}
>>> void st_arg (A a, A *p) {*p = a;}
>>>
>>> For this patch, bootstrap and regtest pass on ppc64{,le}
>>> and x86_64.
>>> * Besides asking for help reviewing this patch, I would like to
>>> consult comments about enhancing for "assigning to returns".
>> I updated the patch to fix the issue for returns.  This patch
>> adds a flag DECL_USEDBY_RETURN_P to indicate if a var is used
>> by a return stmt.  This patch fix the issue in expand pass only,
>> so, we would try to update the patch to avoid this flag.
>>
>> diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
>> index dd29ffffc03..09b8ec64cea 100644
>> --- a/gcc/cfgexpand.cc
>> +++ b/gcc/cfgexpand.cc
>> @@ -2158,6 +2158,20 @@ expand_used_vars (bitmap forced_stack_vars)
>>       frame_phase = off ? align - off : 0;
>>     }
>>   +  /* Collect VARs on returns.  */
>> +  if (DECL_RESULT (current_function_decl))
>> +    {
>> +      edge_iterator ei;
>> +      edge e;
>> +      FOR_EACH_EDGE (e, ei, EXIT_BLOCK_PTR_FOR_FN (cfun)->preds)
>> +	if (greturn *ret = safe_dyn_cast<greturn *> (last_stmt (e->src)))
>> +	  {
>> +	    tree val = gimple_return_retval (ret);
>> +	    if (val && VAR_P (val))
>> +	      DECL_USEDBY_RETURN_P (val) = 1;
>> +	  }
>> +    }
>> +
>>     /* Set TREE_USED on all variables in the local_decls.  */
>>     FOR_EACH_LOCAL_DECL (cfun, i, var)
>>       TREE_USED (var) = 1;
>> diff --git a/gcc/expr.cc b/gcc/expr.cc
>> index d9407432ea5..20973649963 100644
>> --- a/gcc/expr.cc
>> +++ b/gcc/expr.cc
>> @@ -6045,6 +6045,52 @@ expand_assignment (tree to, tree from, bool nontemporal)
>>         return;
>>       }
>>   +  if ((TREE_CODE (from) == PARM_DECL && DECL_INCOMING_RTL (from)
>> +       && TYPE_MODE (TREE_TYPE (from)) == BLKmode
>> +       && (GET_CODE (DECL_INCOMING_RTL (from)) == PARALLEL
>> +	   || REG_P (DECL_INCOMING_RTL (from))))
>> +      || (VAR_P (to) && DECL_USEDBY_RETURN_P (to)
>> +	  && TYPE_MODE (TREE_TYPE (to)) == BLKmode
>> +	  && GET_CODE (DECL_RTL (DECL_RESULT (current_function_decl)))
>> +	       == PARALLEL))
>> +    {
>> +      push_temp_slots ();
>> +      rtx par_ret;
>> +      machine_mode mode;
>> +      par_ret = TREE_CODE (from) == PARM_DECL
>> +		  ? DECL_INCOMING_RTL (from)
>> +		  : DECL_RTL (DECL_RESULT (current_function_decl));
>> +      mode = GET_CODE (par_ret) == PARALLEL
>> +	       ? GET_MODE (XEXP (XVECEXP (par_ret, 0, 0), 0))
>> +	       : word_mode;
>> +      int mode_size = GET_MODE_SIZE (mode).to_constant ();
>> +      int size = INTVAL (expr_size (from));
>> +
>> +      /* If/How the parameter using submode, it dependes on the size and
>> +	 position of the parameter.  Here using heurisitic number.  */
>> +      int hurstc_num = 8;
>
> Where did this come from and what does it mean?
Sorry for does not make this clear. We know that an aggregate arg may be
on registers partially or totally, as assign_parm_adjust_entry_rtl.
For an example, if a parameter with 12 words and the target/ABI only
allow 8 gprs for arguments, then the parameter could use 8 regs at most
and left part in stack.

>
>
> Note that BLKmode subword values passed in registers can be either
> right or left justified.  I think you also need to worry about
> endianness here.
Since the subword is used to move block(read from source mem and then
store to destination mem with register mode), and this would keep to use
the same endianness on reg like move_block_from_reg. So, the patch does
not check the endianness.

If any concerns and sugguestions, please point out, thanks!

BR,
Jeff (Jiufu)
>
>
> Jeff