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

[Hsiangkai Wang via Phabricator via llvm-commits]

HsiangKai added a comment.

Some suggestions inlined.



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:323
     : RVInstI<funct3, OPC_SYSTEM, (outs GPR:$rd), (ins csr_sysreg:$imm12, GPR:$rs1),
-              opcodestr, "$rd, $imm12, $rs1">;
+              opcodestr, "$rd, $imm12, $rs1">, Sched<[WriteCSR]>;
 
----------------
Specify SchedReadWrite for $rs1.


================
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]>;
 
----------------
If all BranchCC_rri have the same Sched<> list, it could be specified in BranchCC_rri class.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:380
+def LBU : Load_ri<0b100, "lbu">, Sched<[WriteLDB]>;
+def LHU : Load_ri<0b101, "lhu">, Sched<[WriteLDH]>;
 
----------------
I supposed you should specify each operand with SchedReadWrite type. It will be the interface between instructions and scheduling model. So, I think we should preserve the place holder for input operands. It will look like

```def LB : Load_ri<0b000, "lb">, Sched<[WriteLDB, ReadLDB]>;```

All the following instructions should specify the SchedReadWrite types for each operand, no matter output operand or input operand.


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedule.td:105
+def ReadIM	  : SchedRead;	// 32 & 64-bit multiply
+def ReadID	  : SchedRead;	// 32 & 64-bit divide
----------------
They are the only two SchedRead definitions. However, there is no place using them. I think you should specify more SchedRead types and associate these SchedRead to input operands in instruction definitions.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68685