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

[Evandro Menezes via Phabricator via llvm-commits]

evandro added a comment.

Except for a handful of gnats, this is really close to LGTM.

- The stores also need a `SchedRead` for register being stored, besides one for the base address register.
- `nop`, both I and C, should have a `ShedWrite` to capture their µop, as some µarchitectures might subsume the µop in the front end.
- The `Pseudo` class definition in `RISCVInstrFormats.td` should set the `SchedRW` to an empty list `[]` so that you wouldn't have to include any `SchedReadWrite` class for pseudo instructions.



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:403
 
-def SB : Store_rri<0b000, "sb">;
-def SH : Store_rri<0b001, "sh">;
-def SW : Store_rri<0b010, "sw">;
+def SB : Store_rri<0b000, "sb">, Sched<[WriteSTB]>;
+def SH : Store_rri<0b001, "sh">, Sched<[WriteSTH]>;
----------------
Stores also read the base address in a register.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:498
+def LD    : Load_ri<0b011, "ld">, Sched<[WriteLDD, ReadLDD]>;
+def SD    : Store_rri<0b011, "sd">, Sched<[WriteSTD]>;
 
----------------
Read base address in register.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoC.td:330
+def C_FSD  : CStore_rri<0b101, "c.fsd", FPR64C, uimm8_lsb000>,
+             Sched<[WriteFST64]> {
   bits<8> imm;
----------------
Again, the base address is in an input register.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoC.td:364
+def C_NOP : RVInst16CI<0b000, 0b01, (outs), (ins), "c.nop", "">,
+            Sched<[]>
 {
----------------
This should use the same `SchedReadWrite` class as for the I `nop`.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoC.td:533
 def C_JALR : RVInst16CR<0b1001, 0b10, (outs), (ins GPRNoX0:$rs1),
-                        "c.jalr", "$rs1">;
+                        "c.jalr", "$rs1">, Sched<[WriteJalr]>;
 
----------------
Input register needs a `SchedRead`.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoC.td:592
 def C_NOP_HINT : RVInst16CI<0b000, 0b01, (outs), (ins simm6nonzero:$imm),
-                            "c.nop", "$imm"> {
+                            "c.nop", "$imm">, Sched<[]> {
   let Inst{6-2} = imm{4-0};
----------------
Same `ShedReadWrite` class as for the I `nop`.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoD.td:82
                   (ins FPR64:$rs2, GPR:$rs1, simm12:$imm12),
-                   "fsd", "$rs2, ${imm12}(${rs1})">;
+                   "fsd", "$rs2, ${imm12}(${rs1})">, Sched<[WriteFST64]>;
 
----------------
`SchedRead`s missing.


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedule.td:126
+def ReadFLD32       : SchedRead;
+def ReadFLD64       : SchedRead;
+def ReadFCvtF32ToI32     : SchedRead;
----------------
A single `SchedRead` for the base address register should suffice for RV32 and RV64 loads **and** stores.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68685