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/22/22 19:58, Jiufu Guo wrote:
>> 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.
cut...
>>>> +	       : 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.
>
> Right, but the number of registers is target dependent, so I don't see
> how using "8" or any number of that matter is correct here.
I understand.  And even for the same struct type, using how many
registers to pass a parameter, it also dependends on the size of the
parameter and how many leading parameters there is.
So, as you said, "8" or any numbers are not always accurate.

Because, the enhancement in this patch is just make "block move" to be
more friendly for follow optiomizations(cse/dse/dce...) by moving
through sub-mode.  So, I just selected one arbitrary number which may
not too large and also tolerable.
I also through to query the max number registers from targets for
param/ret passing, but it may not very accurate neither.

Any sugguestions are welcome! and thanks!

>
>
>>
>>>
>>> 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.
>
> Hmm, I was clear enough here, particularly using the endianness term. 
> Don't you need to query the ABI to ensure that you're not changing
> left vs right justification for a partially in register argument.    
> On the PA we have:
>
> /* Specify padding for the last element of a block move between registers
>    and memory.
>
>    The 64-bit runtime specifies that objects need to be left justified
>    (i.e., the normal justification for a big endian target).  The 32-bit
>    runtime specifies right justification for objects smaller than 64 bits.
>    We use a DImode register in the parallel for 5 to 7 byte structures
>    so that there is only one element.  This allows the object to be
>    correctly padded.  */
> #define BLOCK_REG_PADDING(MODE, TYPE, FIRST) \
>   targetm.calls.function_arg_padding ((MODE), (TYPE))

Yes. We should be careful when store registers to stack
(assign_parms/assign_parm_setup_xx/block/reg), or load to returns.

For this patch, only simple stuffs are handled like "D.xxx = param_1",
where the source and dest of the assignment are all in memory which is
the DECL_RTL(of D.xx/param_xx) in MEM_P/BLK.
And to avoid complicate, this patch only handle the case where
"(size % mode_size) == 0".

If any misunderstandings, please point out, thanks.
And thanks for comments! 


BR,
Jeff (Jiufu)

>
>
> Jeff