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-29 11:05  Kito Cheng <kito.cheng@gmail.com> wrote:
>
>Thanks for this patch, just few minor comment, I think this is pretty
>close to accept :)
>
>Could you reference JiaWei's match_parallel[1] to prevent adding bunch
>of *_offset_operand and stack_push_up_to_*_operand?
>
>
>[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.

>
>
>> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>> index 629e5e45cac..a0a2db1f594 100644
>> --- a/gcc/config/riscv/riscv.cc
>> +++ b/gcc/config/riscv/riscv.cc
>> @@ -117,6 +117,14 @@ struct GTY(())  riscv_frame_info {
>>    /* How much the GPR save/restore routines adjust sp (or 0 if unused).  */
>>    unsigned save_libcall_adjustment;
>>
>> +  /* the minimum number of bytes, in multiples of 16-byte address increments,
>> +     required to cover the registers in a multi push & pop.  */
>> +  unsigned multi_push_adj_base;
>> +
>> +  /* the number of additional 16-byte address increments allocated for the stack frame
>> +     in a multi push & pop.  */
>> +  unsigned multi_push_adj_addi;
>> +
>>    /* Offsets of fixed-point and floating-point save areas from frame bottom */
>>    poly_int64 gp_sp_offset;
>>    poly_int64 fp_sp_offset;
>> @@ -413,6 +421,21 @@ static const struct riscv_tune_info riscv_tune_info_table[] = {
>>  #include "riscv-cores.def"
>>  };
>>
>> +typedef enum
>> +{
>> +  SI_IDX = 0,
>> +  DI_IDX,
>> +  MAX_MODE_IDX = DI_IDX
>> +} mode_idx;
>> +
>
>Didn't see any use in this version? 
It's used in defining the array below.
const insn_gen_fn gen_push_pop [MAX_OP_IDX + 1][MAX_MODE_IDX + 1][ZCMP_MAX_GRP_SLOTS]

>
>> @@ -5574,18 +5924,25 @@ riscv_expand_epilogue (int style)
>>        REG_NOTES (insn) = dwarf;
>>      }
>>
>> -  if (use_restore_libcall)
>> -    frame->mask = 0; /* Temporarily fib for GPRs.  */
>> +  if (use_restore_libcall || use_multi_pop)
>> +    frame->mask = 0; /* Temporarily fib that we need not save GPRs.  */
>>
>>    /* If we need to restore registers, deallocate as much stack as
>>       possible in the second step without going out of range.  */
>> -  if ((frame->mask | frame->fmask) != 0)
>> +  if (use_multi_pop)
>> +    {
>> +      if (frame->fmask
>> +          && known_gt (frame->total_size - multipop_size,
>> +                      frame->frame_pointer_offset))
>> +        step2 = riscv_first_stack_step (frame, frame->total_size - multipop_size);
>> +    }
>> +  else if ((frame->mask | frame->fmask) != 0)
>>      step2 = riscv_first_stack_step (frame, frame->total_size - libcall_size);
>>
>> -  if (use_restore_libcall)
>> +  if (use_restore_libcall || use_multi_pop)
>>      frame->mask = mask; /* Undo the above fib.  */
>>
>> -  poly_int64 step1 = frame->total_size - step2 - libcall_size;
>> +  poly_int64 step1 = frame->total_size - step2 - libcall_size - multipop_size ;
>>
>>    /* Set TARGET to BASE + STEP1.  */
>>    if (known_gt (step1, 0))
>> @@ -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.

>
>>
>>           dwarf = alloc_reg_note (REG_CFA_DEF_CFA, cfa_adjust_rtx, dwarf);
>>           RTX_FRAME_RELATED_P (insn) = 1;
>> @@ -5635,15 +5992,15 @@ riscv_expand_epilogue (int style)
>>        epilogue_cfa_sp_offset = step2;
>>      }
>>
>> -  if (use_restore_libcall)
>> +  if (use_restore_libcall || use_multi_pop)
>>      frame->mask = 0; /* Temporarily fib that we need not save GPRs.  */
>>
>>    /* Restore the registers.  */
>> -  riscv_for_each_saved_reg (frame->total_size - step2 - libcall_size,
>> +  riscv_for_each_saved_reg (frame->total_size - step2 - libcall_size - multipop_size,
>>                             riscv_restore_reg,
>>                             true, style == EXCEPTION_RETURN);
>>
>> -  if (use_restore_libcall)
>> +  if (use_restore_libcall || use_multi_pop)
>>        frame->mask = mask; /* Undo the above fib.  */
>>
>>    if (need_barrier_p)
>> @@ -5657,14 +6014,30 @@ riscv_expand_epilogue (int style)
>>
>>        rtx dwarf = NULL_RTX;
>>        rtx cfa_adjust_rtx = gen_rtx_PLUS (Pmode, stack_pointer_rtx,
>> -                                        const0_rtx);
>> +                                        GEN_INT (libcall_size + multipop_size));
>
>Same question for `libcall_size` part. 

Same reason as described above.

BR, 
Fei