Re: Re: [PATCH 1/1] [V2] [RISC-V] support cm.push cm.pop cm.popret in zcmp

["Fei Gao" <gaofei@eswincomputing.com>]

On 2023-05-31 18:13  Kito Cheng <kito.cheng@gmail.com> wrote:
>
>> >[1] https://patchwork.sourceware.org/project/gcc/patch/20230406062118.47431-5-jiawei@iscas.ac.cn/
>> Thanks for your review.
>>
>> The md file looks verbose with bunch of *_offset_operand and stack_push_up_to_*_operand, but it significantly
>> simplies implementation of recognizing zmcp push and pop insns and outputting assembly.  Also, the md file
>> clearly shows and checks the slot that each register is placed(different to slot order w/o save-restore before
>> zcmp is introduced). So I prefer my patch V2 to V1 or the link you attached. But ideas are welcome to make
>> it better. Appreciated if you suggest more details for the improvement.
>
>Got your point, and share an idea to simplify that:
>
>struct code_for_push_pop_t {
>   insn_code (*push)(machine_mode);
>   insn_code (*pop)(machine_mode);
>   insn_code (*pop_ret)(machine_mode);
>};
>const code_for_push_pop_t code_for_push_pop [/*ZCMP_MAX_GRP_SLOTS*/2] = {
>    {code_for_gpr_multi_pop_up_to_ra, /*FIXME*/nullptr, /*FIXME*/nullptr},
>    {code_for_gpr_multi_pop_up_to_s0, /*FIXME*/nullptr, /*FIXME*/nullptr}
>};
>
>static rtx
>riscv_gen_multi_push_pop_insn (op_idx op, HOST_WIDE_INT adj_size,
>unsigned int regs_num)
>{
>  rtx stack_adj = GEN_INT (adj_size);
>
>  return GEN_FCN (code_for_push_pop[regs_num].push(Pmode)) (stack_adj);
>}
>
>(define_mode_attr slot0_offset [(SI "0") (DI "0")])
>(define_mode_attr slot1_offset [(SI "4") (DI "8")])
>
>(define_insn "@gpr_multi_pop_up_to_ra<mode>"
>  [(set (reg:X SP_REGNUM)
>        (plus:X (reg:X SP_REGNUM)
>                 (match_operand 0 "stack_pop_up_to_ra_operand" "I")))
>   (set (reg:X RETURN_ADDR_REGNUM)
>        (mem:X (plus:X (reg:X SP_REGNUM)
>                       (const_int <slot0_offset>))))]
>  "TARGET_ZCMP"
>  "cm.pop       {ra}, %0"
>)
>
>(define_insn "@gpr_multi_pop_up_to_s0<mode>"
>  [(set (reg:X SP_REGNUM)
>        (plus:X (reg:X SP_REGNUM)
>                 (match_operand 0 "stack_pop_up_to_s0_operand" "I")))
>   (set (reg:X S0_REGNUM)
>        (mem:X (plus:X (reg:X SP_REGNUM)
>                       (const_int <slot0_offset>))))
>   (set (reg:X RETURN_ADDR_REGNUM)
>        (mem:X (plus:X (reg:X SP_REGNUM)
>                       (const_int <slot1_offset>))))]
>  "TARGET_ZCMP"
>  "cm.pop       {ra, s0}, %0"
>) 

Perfect. 
Working on it. 

>
>
>
>> >> @@ -5620,7 +5977,7 @@ riscv_expand_epilogue (int style)
>> >>                                            adjust));
>> >>           rtx dwarf = NULL_RTX;
>> >>           rtx cfa_adjust_rtx = gen_rtx_PLUS (Pmode, stack_pointer_rtx,
>> >> -                                            GEN_INT (step2));
>> >> +                                            GEN_INT (step2 + libcall_size + multipop_size));
>> >
>> >Why we need `+ libcall_size` here? or...why we don't need that before?
>> It's a good catch:)
>> I should have  added `+ libcall_size` in
>> https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=60524be1e3929d83e15fceac6e2aa053c8a6fb20
>>
>> That's why I corrected the cfi issue in save-restore along with zcmp changes in this patch.
>
>I would like to have a separate patch to fix this bug instead of
>hidden in this patch. 
sure,  I will make  a separate patch. 

Thanks & BR, 
Fei