[PATCH] D68685: [RISCV] Scheduler description for Rocket Core

[Evandro Menezes via Phabricator via llvm-commits]

evandro added a comment.

Overall, it looks nearly good enough.  I don't know how the scheduler will behave is interesting cases when there is no `ShedRead` for each argument in the `ins` list of the instructions.  However, it shouldn't be too difficult to add them in the instruction classes.



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:374
+def BLTU : BranchCC_rri<0b110, "bltu">, Sched<[WriteJmp]>;
+def BGEU : BranchCC_rri<0b111, "bgeu">, Sched<[WriteJmp]>;
 
----------------
HsiangKai wrote:
> If all BranchCC_rri have the same Sched<> list, it could be specified in BranchCC_rri class.
Agreed.


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedRocket32.td:31
+def Rocket32UnitIMul       : ProcResource<1>; // Int Multiply
+def Rocket32UnitIDiv       : ProcResource<1>; // Int Division
+def Rocket32UnitMem        : ProcResource<1>; // Load/Store
----------------
You might consider setting `BufferSize = 1` for this non-pipelined units.


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedRocket32.td:36
+def Rocket32UnitFPALU      : ProcResource<1>; // FP ALU
+def Rocket32UnitFPDivSqrt  : ProcResource<1>; // FP Divide/Sqrt
+}
----------------
You might consider setting `BufferSize = 1` for this non-pipelined units.


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedRocket32.td:60
+// 32-bit divides have worse case latency of 34 cycle
+def : WriteRes<WriteIDiv, [Rocket32UnitIMul]> { let Latency = 17; }
+
----------------
Typically, for most values, the latency is near the worst case.  I suggest using the worst case here.  And, as @javed.absar said, it also needs `ResourceCycle = <worst case>;` here as well.  


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedRocket64.td:30
+def Rocket64UnitIMul       : ProcResource<1>; // Int Multiply
+def Rocket64UnitIDiv       : ProcResource<1>; // Int Division
+def Rocket64UnitMem        : ProcResource<1>; // Load/Store
----------------
You might consider setting `BufferSize = 1` for this non-pipelined units.


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedRocket64.td:35
+def Rocket64UnitFPALU      : ProcResource<1>; // FP ALU
+def Rocket64UnitFPDivSqrt  : ProcResource<1>; // FP Divide/Sqrt
+}
----------------
You might consider setting `BufferSize = 1` for this non-pipelined units.


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedRocket64.td:59
+
+// Integer divide varies based on operand magnitude and sign; worse case latency is 34.
+def : WriteRes<WriteIDiv32, [Rocket64UnitIDiv]> { let Latency = 17; }
----------------
Again, I suggest using the worst case here and below.


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedRocket64.td:100
+// Most FP double precision operations are 5 cycles
+def : WriteRes<WriteFALU64, [Rocket64UnitFPALU]> { let Latency = 6; }
+
----------------
Per the comment above, is it 5 or 6?


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedRocket64.td:125
+
+def : WriteRes<WriteFMul32, [Rocket64UnitFPALU]> { let Latency = 5; }
+def : WriteRes<WriteFMulAdd32, [Rocket64UnitFPALU]> { let Latency = 5; }
----------------
Outline the `Latency` to avoid repetitions.


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedRocket64.td:128
+def : WriteRes<WriteFMulSub32, [Rocket64UnitFPALU]> { let Latency = 5; }
+def : WriteRes<WriteFMul64, [Rocket64UnitFPALU]> { let Latency = 7; }
+def : WriteRes<WriteFMulAdd64, [Rocket64UnitFPALU]> { let Latency = 7; }
----------------
Ditto.


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedule.td:22
+def WriteFence      : SchedWrite;    // Fence instructions
+def WriteNop        : SchedWrite;
+def WriteLDB        : SchedWrite;    // Load byte
----------------
I don't think that `nop` needs a scheduling class.


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedule.td:27
+def WriteLDH        : SchedWrite;    // Load half-word
+def WriteLDDW       : SchedWrite;    // Load double-word
+def WriteCSR        : SchedWrite;    // CSR instructions
----------------
```
s/WriteLDDW/WriteLDD/
```


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedule.td:32
+def WriteSTH        : SchedWrite;    // Store half-word
+def WriteSTDW       : SchedWrite;    // Store double-word
+def WriteAtomicW    : SchedWrite;    //Atomic memory operation word size
----------------
```
s/WriteLSTDW/WriteSTD/
```


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedule.td:36
+def WriteAtomicLDW  : SchedWrite;    // Atomic load word
+def WriteAtomicLDDW : SchedWrite;    // Atomic load double word
+def WriteAtomicSTW  : SchedWrite;    // Atomic store word
----------------
```
s/WriteAtomicLDDW/WriteAtomicLDD/
```


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedule.td:38
+def WriteAtomicSTW  : SchedWrite;    // Atomic store word
+def WriteAtomicSTDW : SchedWrite;    // Atomic store double word
+def WriteTrapRet    : SchedWrite;    // Trap return instructions
----------------
```
s/WriteAtomicSTDW/WriteAtomicSTD/
```


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedule.td:57
+def WriteFCvtI64ToF64     : SchedWrite;    // RV64I only
+def WriteFCvtI64ToF32     : SchedWrite;    // RV64I only
+
----------------
Swap this line with the one above it.


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedule.td:83
+def WriteFMov64           : SchedWrite;    // 64-bit floating point move
+def WriteFLd32            : SchedWrite;    // Floating point sp load
+def WriteFLd64            : SchedWrite;    // Floating point dp load
----------------
```
s/WriteFLd32/WriteFLD32/
```


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedule.td:84
+def WriteFLd32            : SchedWrite;    // Floating point sp load
+def WriteFLd64            : SchedWrite;    // Floating point dp load
+def WriteFSt32            : SchedWrite;    // Floating point sp store
----------------
```
s/WriteFLd64/WriteFLD64/
```


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedule.td:85
+def WriteFLd64            : SchedWrite;    // Floating point dp load
+def WriteFSt32            : SchedWrite;    // Floating point sp store
+def WriteFSt64            : SchedWrite;    // Floating point dp store
----------------
```
s/WriteFSt32s/WriteFST32/
```


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedule.td:86
+def WriteFSt32            : SchedWrite;    // Floating point sp store
+def WriteFSt64            : SchedWrite;    // Floating point dp store
----------------
```
s/WriteFSt64/WriteFST64/
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68685/new/

https://reviews.llvm.org/D68685