Re: [PATCH 1/3][RFC] RISC-V: Add non-vector types to pipelines

[Edwin Lu <ewlu@rivosinc.com>]

On 12/20/2023 10:50 AM, Jeff Law wrote:
> 
> 
> On 12/15/23 11:53, Edwin Lu wrote:
>> This patch does not create vector related insn reservations for
>> generic.md and sifive-7.md. It updates/creates insn reservations
>> for all non-vector typed insns
>>
>> gcc/ChangeLog:
>>
>>     * config/riscv/generic-ooo.md (generic_ooo_sfb_alu): create/update 
>> reservation
>>     (generic_ooo_branch): ditto
>>     * config/riscv/generic.md (generic_sfb_alu): ditto
>>     * config/riscv/sifive-7.md (sifive_7_popcount): ditto
>>
>> Signed-off-by: Edwin Lu <ewlu@rivosinc.com>
>> ---
>>   gcc/config/riscv/generic-ooo.md | 16 +++++++++++++---
>>   gcc/config/riscv/generic.md     | 13 +++++++++----
>>   gcc/config/riscv/sifive-7.md    | 12 +++++++++---
>>   3 files changed, 31 insertions(+), 10 deletions(-)
>>
>> diff --git a/gcc/config/riscv/generic-ooo.md 
>> b/gcc/config/riscv/generic-ooo.md
>> index 78b9e48f935..de93245f965 100644
>> --- a/gcc/config/riscv/generic-ooo.md
>> +++ b/gcc/config/riscv/generic-ooo.md
>> @@ -95,7 +95,7 @@ (define_insn_reservation "generic_ooo_float_store" 6
>>   ;; Vector load/store
>>   (define_insn_reservation "generic_ooo_vec_load" 6
>>     (and (eq_attr "tune" "generic_ooo")
>> -       (eq_attr "type" "vlde,vldm,vlds,vldux,vldox,vldff,vldr"))
>> +       (eq_attr "type" "vlde,vldm,vlds,vldux,vldox,vldff,vldr,rdfrm"))
>>     "generic_ooo_vxu_issue,generic_ooo_vxu_alu")
> Hmm, I don't think "rdfrm" is really a vector load.  It's really a read 
> of some bits in the fpcsr which elsewhere is handled as type fmove.  I'd 
> actually suggest fixing vector.md to use fmove on the appropriate insn, 
> then dropping rdfrm entirely.
> Sounds good!
> 
> 
>>   (define_insn_reservation "generic_xfer" 3
>>     (and (eq_attr "tune" "generic")
>> -       (eq_attr "type" "mfc,mtc,fcvt,fmove,fcmp"))
>> +       (eq_attr "type" "mfc,mtc,fcvt,fmove,fcmp,cbo"))
>>     "alu")
> cbo is probably closer to a load/store than it is a transfer operation.
> 
That makes sense. I wasn't sure where exactly to put it since we have 
two separate insn reservations for load and store and from my 
understanding, the same type cannot be in two separate insn 
reservations. Would a new insn reservation like
(define_insn_reservation "generic_load_store" 2 ...) work?

>>   (define_insn_reservation "generic_branch" 1
>>     (and (eq_attr "tune" "generic")
>> -       (eq_attr "type" "branch,jump,call,jalr"))
>> +       (eq_attr "type" "branch,jump,call,jalr,ret,trap,pushpop"))
>> +  "alu")
> pushpop are a mix of some pure memory operations and some mixed memory + 
> branch.
> 
> However, from a scheduling standpoint the branch isn't particularly 
> interesting.  So I'd have pushpop as a load/store.
> 
Same as above

Edwin