[PATCH] D90853: [RISCV] Add DAG nodes to represent read/write CSR

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff marked an inline comment as done.
sepavloff added a comment.

In D90853#2376736 <https://reviews.llvm.org/D90853#2376736>, @jrtc27 wrote:

> What's the motivation for this?

I am working on the implementation of SET_ROUNDIND on RISCV, it is D91242 <https://reviews.llvm.org/D91242>. To set rounding mode it is sufficient to write a proper value to corresponding CSR. CSRRW or CSRRS may be used for that but they set output register. It is not clear how to set a physical register (X0) as an output without making a new class. Moreover it is incorrect to have output register at all in this case because such instruction does not set any register. As read-write and write-only instructions differ in number of outputs, they must be different instructions in MIR. In contrast to them read-only instructions do not need separate MachineInstr as X0 may be specified as input operand.



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:397
+    hasSideEffects = 1, mayLoad = 0, mayStore = 0, isCodeGenOnly = 1 in
+class CSR_ir_wo<bits<3> funct3, string opcodestr>
+    : RVInstI<funct3, OPC_SYSTEM, (outs), (ins csr_sysreg:$imm12, GPR:$rs1),
----------------
jrtc27 wrote:
> CSRW_ir might be a better name (though CSRW could be confused with the pseudoinstruction), not a huge fan of _wo.
Changed to `CSRW_*`.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:399
+    : RVInstI<funct3, OPC_SYSTEM, (outs), (ins csr_sysreg:$imm12, GPR:$rs1),
+              opcodestr, "$imm12, $rs1">, Sched<[WriteCSR, ReadCSR]> {
+  let rd = 0;
----------------
jrtc27 wrote:
> This one has ReadCSR but CSR_ii_wo doesn't; which is it?
Yes, updated it.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1198-1203
+def : Pat<(riscv_read_csr simm12:$csr),
+          (CSRRS simm12:$csr, X0)>;
+def : Pat<(riscv_write_csr simm12:$csr, GPR:$rs1),
+          (CSRW simm12:$csr, GPR:$rs1)>;
+def : Pat<(riscv_write_csr simm12:$csr, uimm5:$imm),
+          (CSRW simm12:$csr, uimm5:$imm)>;
----------------
jrtc27 wrote:
> Why do we need CSR_i[ir]_wo but not CSR_i[ir]_ro? Either both are needed if you need to be able to specify scheduling for instructions that only do one of read or write, or you don't need either of them, surely?
> Why do we need CSR_i[ir]_wo but not CSR_i[ir]_ro?
We can specify `X0` as an input operand, but it seems there is no simple way to specify `X0` as output. 

> Either both are needed if you need to be able to specify scheduling for instructions that only do one of read or write

It depends on the implementation of the scheduler. Scheduling probably does not depend on the output register, as the hardware instruction in both cases is the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90853