diff mbox series

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

Message ID 20231215185328.794425-2-ewlu@rivosinc.com
State Unresolved
Headers
Series RISC-V: Associate typed insns to dfa reservation |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Edwin Lu Dec. 15, 2023, 6:53 p.m. UTC 
  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(-)

Comments

Jeff Law Dec. 20, 2023, 6:50 p.m. UTC | #1 
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.



>   
>   (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.

>   
>   (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.


jeff
Edwin Lu Dec. 20, 2023, 10:11 p.m. UTC | #2 
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
Jeff Law Dec. 21, 2023, 6:59 a.m. UTC | #3 
On 12/20/23 15:11, Edwin Lu wrote:

>>
>>
>>>   (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?
I'd probably just treat cbo instructions as stores.  In fact, you could 
probably get away with using "store" as the type and dropping the cbo 
type entirely.




> 
>>>   (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.
So for these I think those which do pushes you could legitimately change 
  to the "store" type and pops could be changed to a "load" type.  If 
something does both just call it a load.  That's going to be the most 
useful from a scheduling standpoint I suspect.



Jeff
diff mbox series

Patch

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")
 
 (define_insn_reservation "generic_ooo_vec_store" 6
@@ -115,9 +115,19 @@  (define_insn_reservation "generic_ooo_vec_loadstore_seg" 10
 (define_insn_reservation "generic_ooo_alu" 1
   (and (eq_attr "tune" "generic_ooo")
        (eq_attr "type" "unknown,const,arith,shift,slt,multi,auipc,nop,logical,\
-			move,bitmanip,min,max,minu,maxu,clz,ctz"))
+			move,bitmanip,rotate,min,max,minu,maxu,clz,ctz,atomic,condmove,cbo,mvpair,zicond"))
   "generic_ooo_issue,generic_ooo_ixu_alu")
 
+(define_insn_reservation "generic_ooo_sfb_alu" 2
+  (and (eq_attr "tune" "generic_ooo")
+       (eq_attr "type" "sfb_alu"))
+  "generic_ooo_issue,generic_ooo_ixu_alu")
+
+;; Branch instructions
+(define_insn_reservation "generic_ooo_branch" 1
+  (and (eq_attr "tune" "generic_ooo")
+       (eq_attr "type" "branch,jump,call,jalr,ret,trap,pushpop"))
+  "generic_ooo_issue,generic_ooo_ixu_alu")
 
 ;; Float move, convert and compare.
 (define_insn_reservation "generic_ooo_float_move" 3
@@ -184,7 +194,7 @@  (define_insn_reservation "generic_ooo_popcount" 2
 (define_insn_reservation "generic_ooo_vec_alu" 3
   (and (eq_attr "tune" "generic_ooo")
        (eq_attr "type" "vialu,viwalu,vext,vicalu,vshift,vnshift,viminmax,vicmp,\
-		        vimov,vsalu,vaalu,vsshift,vnclip,vmov,vfmov"))
+		        vimov,vsalu,vaalu,vsshift,vnclip,vmov,vfmov,vector"))
   "generic_ooo_vxu_issue,generic_ooo_vxu_alu")
 
 ;; Vector float comparison, conversion etc.
diff --git a/gcc/config/riscv/generic.md b/gcc/config/riscv/generic.md
index 88940483829..3e49d942495 100644
--- a/gcc/config/riscv/generic.md
+++ b/gcc/config/riscv/generic.md
@@ -27,7 +27,7 @@  (define_cpu_unit "fdivsqrt" "pipe0")
 
 (define_insn_reservation "generic_alu" 1
   (and (eq_attr "tune" "generic")
-       (eq_attr "type" "unknown,const,arith,shift,slt,multi,auipc,nop,logical,move,bitmanip,min,max,minu,maxu,clz,ctz,cpop"))
+       (eq_attr "type" "unknown,const,arith,shift,slt,multi,auipc,nop,logical,move,bitmanip,min,max,minu,maxu,clz,ctz,rotate,atomic,condmove,crypto,mvpair,zicond"))
   "alu")
 
 (define_insn_reservation "generic_load" 3
@@ -42,17 +42,22 @@  (define_insn_reservation "generic_store" 1
 
 (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")
 
 (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")
+
+(define_insn_reservation "generic_sfb_alu" 2
+  (and (eq_attr "tune" "generic")
+       (eq_attr "type" "sfb_alu"))
   "alu")
 
 (define_insn_reservation "generic_imul" 10
   (and (eq_attr "tune" "generic")
-       (eq_attr "type" "imul,clmul"))
+       (eq_attr "type" "imul,clmul,cpop"))
   "imuldiv*10")
 
 (define_insn_reservation "generic_idivsi" 34
diff --git a/gcc/config/riscv/sifive-7.md b/gcc/config/riscv/sifive-7.md
index a63394c8c58..65d27cf6dc9 100644
--- a/gcc/config/riscv/sifive-7.md
+++ b/gcc/config/riscv/sifive-7.md
@@ -34,7 +34,7 @@  (define_insn_reservation "sifive_7_fpstore" 1
 
 (define_insn_reservation "sifive_7_branch" 1
   (and (eq_attr "tune" "sifive_7")
-       (eq_attr "type" "branch"))
+       (eq_attr "type" "branch,ret,trap"))
   "sifive_7_B")
 
 (define_insn_reservation "sifive_7_sfb_alu" 2
@@ -44,7 +44,7 @@  (define_insn_reservation "sifive_7_sfb_alu" 2
 
 (define_insn_reservation "sifive_7_jump" 1
   (and (eq_attr "tune" "sifive_7")
-       (eq_attr "type" "jump,call,jalr"))
+       (eq_attr "type" "jump,call,jalr,pushpop"))
   "sifive_7_B")
 
 (define_insn_reservation "sifive_7_mul" 3
@@ -59,7 +59,7 @@  (define_insn_reservation "sifive_7_div" 16
 
 (define_insn_reservation "sifive_7_alu" 2
   (and (eq_attr "tune" "sifive_7")
-       (eq_attr "type" "unknown,arith,shift,slt,multi,logical,move"))
+       (eq_attr "type" "unknown,arith,shift,slt,multi,logical,move,bitmanip,rotate,min,max,minu,maxu,clz,ctz,cbo,atomic,condmove,crypto,mvpair,zicond"))
   "sifive_7_A|sifive_7_B")
 
 (define_insn_reservation "sifive_7_load_immediate" 1
@@ -106,6 +106,12 @@  (define_insn_reservation "sifive_7_f2i" 3
        (eq_attr "type" "mfc"))
   "sifive_7_A")
 
+;; Popcount and clmul.
+(define_insn_reservation "sifive_7_popcount" 2
+  (and (eq_attr "tune" "sifive_7")
+       (eq_attr "type" "cpop,clmul"))
+  "sifive_7_A")
+
 (define_bypass 1 "sifive_7_load,sifive_7_alu,sifive_7_mul,sifive_7_f2i,sifive_7_sfb_alu"
   "sifive_7_alu,sifive_7_branch")