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

[sepavloff]

Summary

Two custom DAG nodes, READ_CSR and WRITE_CSR, were added. They represent
read and write operations on CSR.

This change also added additional instruction patterns to represent write-only
variants of CSRRW. They are encoded by putting X0 as destination register, so
actually such instruction does not define its destination register. Generic
read-and-write form of CSRRW is not suitable in this case, as it always
produce produce output value.

[jrtc27]

What's the motivation for this?

llvm/lib/Target/RISCV/RISCVInstrInfo.td

397

CSRW_ir might be a better name (though CSRW could be confused with the pseudoinstruction), not a huge fan of _wo.

399

This one has ReadCSR but CSR_ii_wo doesn't; which is it?

1198–1203

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?

[sepavloff]

Updated patch

[sepavloff]

In D90853#2376736, @jrtc27 wrote:

What's the motivation for this?

I am working on the implementation of SET_ROUNDIND on RISCV, it is 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.

llvm/lib/Target/RISCV/RISCVInstrInfo.td

397

Changed to CSRW_*.

399

Yes, updated it.

1198–1203

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.

[jrtc27]

I’m pretty sure you can use X0 as the output...

[sepavloff]

In D90853#2388346, @jrtc27 wrote:

I’m pretty sure you can use X0 as the output...

Could you please tell me how I can do that?

[sepavloff]

Even if there were a way to specify particular register in outputs, fake writes to X0 would create false output dependencies, which would require specific treatment. Using instructions without output is a natural way to represent such cases.

[jrtc27]

In D90853#2399517, @sepavloff wrote:

Even if there were a way to specify particular register in outputs

You mean make RISCV::X0 the first operand? I don’t understand what the problem is.

, fake writes to X0 would create false output dependencies, which would require specific treatment. Using instructions without output is a natural way to represent such cases.

LLVM knows it’s a constant register. The correct thing to do IMO is fix any places in LLVM that don’t account for constant registers, if there are any, and then have a generic solution, rather than try and work around any deficiencies by adding special cases to backends every single time it comes up.

[sepavloff]

In D90853#2399577, @jrtc27 wrote:

In D90853#2399517, @sepavloff wrote:

, fake writes to X0 would create false output dependencies, which would require specific treatment. Using instructions without output is a natural way to represent such cases.

LLVM knows it’s a constant register. The correct thing to do IMO is fix any places in LLVM that don’t account for constant registers, if there are any, and then have a generic solution, rather than try and work around any deficiencies by adding special cases to backends every single time it comes up.

The fact that X0 is used in destination register field of an instruction to produce write-only variant is peculiarity of RICSV encoding. It does not mean that X0 is defined by the instruction. DAG is a higher layer it tries to abstracts from particular ISA. Results of a DAG node are treated much like results of function calls. So write-only CSR instructions should be represented by different nodes in DAG because they have different number of produced values.

[rogfer01]

Hi Serge, would it make sense to use a Pseudo for those specific cases with a custom inserter? (usesCustomInserter = 1 in the tablegen definition of the Pseudo)

This way you could have PseudoCSRW and PseudoCSRWI (it looks to me you do not need the other cases, did I get that right?) that you can use in the patterns. Then you can expand them to the existing MachineInstructions CSRRW, CSRRWI, respectively (hope I didn't get the names wrong), that use X0 as the destination register in RISCVTargetLowering::EmitInstrWithCustomInserter.

I understand your concern with X0 potentially defining a false write dependency, but I too understand that we should fix any case in LLVM where constant registers are not handled correctly.

[sepavloff]

In D90853#2482536, @rogfer01 wrote:

Hi Serge, would it make sense to use a Pseudo for those specific cases with a custom inserter? (usesCustomInserter = 1 in the tablegen definition of the Pseudo)

This way you could have PseudoCSRW and PseudoCSRWI (it looks to me you do not need the other cases, did I get that right?) that you can use in the patterns. Then you can expand them to the existing MachineInstructions CSRRW, CSRRWI, respectively (hope I didn't get the names wrong), that use X0 as the destination register in RISCVTargetLowering::EmitInstrWithCustomInserter.

You describe how to make X0 an output register. I agree, it look like using custom inserter is the only way to make such instruction. This way is substantially more complex than just adding 3 new auxiliary instructions. But not complexity is the main concern.

Making X0 an output register contradicts with design of both DAG and MIR. DAG node and MachineInstr behave as functions, they have clear distinction of input and output operands. Even if a register is both input and output, it is represented by two operands. Functional nature of DAG and MIR is used in many cases, where use-def chains are examined, like lifetime analysis or loop invariant movement. Adding "definitions" for register that is immutable would break the functional nature and might require multiple changes in various parts of compiler.

I understand your concern with X0 potentially defining a false write dependency, but I too understand that we should fix any case in LLVM where constant registers are not handled correctly.

It creates also true dependencies, any use of X0 after CSR write would depend on the latter. For example, if a loop contains an invariant expression that uses X0, it would not be moved if the loop contains CSR write.

Using X0 as output is just a trick to have a new instruction without spending opcode. Actually such instruction does not define X0. What is the benefit of exposing this low-level encoding feature in high-level structures?

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

565

There is no write only version of CSRS/CSRC/CSRSI/CSRCI. There's a read/write and read only version of those.

[sepavloff]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

565

Indeed. Thank you for the catch. I will remove them.

[rogfer01]

Hi Serge,

Using X0 as output is just a trick to have a new instruction without spending opcode. Actually such instruction does not define X0. What is the benefit of exposing this low-level encoding feature in high-level structures?

My suggestion was to avoid the situation where we have two machine instructions that overlap in their semantics. This entails that a later pass that analyses CSRs should take into account those write only forms in addition to the actual instructions. However, maybe this is not a practical issue. The number of CSR instructions is not large. It may also happen that SelectionDAG will never select a CSR write instruction that writes to X0. Or if it does, we would always use the new write-only form that you suggest.

I'm not sure if we would want to prefix those write-only versions with Pseudo? (like it happens with other instructions that exist for the purpose of Codegen).

[sepavloff]

Removed read-only variants of CSRRC and CSRRS. Rebased.

[sepavloff]

In D90853#2489631, @rogfer01 wrote:

Hi Serge,

Using X0 as output is just a trick to have a new instruction without spending opcode. Actually such instruction does not define X0. What is the benefit of exposing this low-level encoding feature in high-level structures?

My suggestion was to avoid the situation where we have two machine instructions that overlap in their semantics. This entails that a later pass that analyses CSRs should take into account those write only forms in addition to the actual instructions. However, maybe this is not a practical issue. The number of CSR instructions is not large. It may also happen that SelectionDAG will never select a CSR write instruction that writes to X0. Or if it does, we would always use the new write-only form that you suggest.

The new instruction patterns are marked as isCodegenOnly, so they won't appear in MC layer. For example, output of disassembler may not contain them. Any analysis made at this level may be unaware of the new patterns. It is only codegen that must take into account the new pattern. But for it these instructions indeed are different, they have different fundamental properties.

It would be, of course, convenient to have close correspondence between instruction bit representation and instruction object used in internal representation. It is however sometimes not possible and codegens often use isCodegenOnly patterns to cope with gap between encoding and machine IR representation. For example X86 defines different patterns for XOR8 depending on whether it is prefixed with REX prefix. Any analysis made at MachineInstr level need to take into account all variants. And X86 is not an exception.

I'm not sure if we would want to prefix those write-only versions with Pseudo? (like it happens with other instructions that exist for the purpose of Codegen).

Defining isCodegenOnly pattern is a clear and compact solution, Pseudo would require additional code to expand it. Besides it is not clear what could be a replacement in this case. It can't be CSRRW with X0 as defined register.

[sepavloff]

Hi all,

What is the destiny of this patch?

My point is that:

• Using X0 as destination is an encoding trick to save opcode space, there is no sense to expose it to higher levels, like DAG or MIR.
• Machine instruction or DAG node which have X0 as destination register breaks DAG or MIR design, as such instruction actually is not a definitions for X0.

It looks like there is no alternative to dedicated instructions for writes to CSRs.

What do you think?

[craig.topper]

In D90853#2625329, @sepavloff wrote:

Hi all,

What is the destiny of this patch?

My point is that:

• Using X0 as destination is an encoding trick to save opcode space, there is no sense to expose it to higher levels, like DAG or MIR.
• Machine instruction or DAG node which have X0 as destination register breaks DAG or MIR design, as such instruction actually is not a definitions for X0.

AArch64 has a pass that replaces defs with X0, AArch64DeadRegisterDefinitionsPass. This is how a subtract becomes a compare. So it is not unprecedented.

It looks like there is no alternative to dedicated instructions for writes to CSRs.

What do you think?

In order to model set rounding mode, don't we need the write_csr pseudo instruction to have an implicit def of FRMReg?

[sepavloff]

In D90853#2625350, @craig.topper wrote:

In D90853#2625329, @sepavloff wrote:

My point is that:

• Using X0 as destination is an encoding trick to save opcode space, there is no sense to expose it to higher levels, like DAG or MIR.
• Machine instruction or DAG node which have X0 as destination register breaks DAG or MIR design, as such instruction actually is not a definitions for X0.

AArch64 has a pass that replaces defs with X0, AArch64DeadRegisterDefinitionsPass. This is how a subtract becomes a compare. So it is not unprecedented.

Interesting pass. Thank you for the reference.

It sets xzr as destination and marks it as dead. The pass is executed relatively late, just before register allocator. Probably RISC-V could use similar technique.

In D90853#2625350, @craig.topper wrote:

In order to model set rounding mode, don't we need the write_csr pseudo instruction to have an implicit def of FRMReg?

There are many system registers, putting them all to implicit definitions of write_csr does not look a flexible solution. I think about setting implicit definitions manually, using addOperand somewhere after selection.

[craig.topper]

In D90853#2629528, @sepavloff wrote:

In D90853#2625350, @craig.topper wrote:

In D90853#2625329, @sepavloff wrote:

My point is that:

• Using X0 as destination is an encoding trick to save opcode space, there is no sense to expose it to higher levels, like DAG or MIR.
• Machine instruction or DAG node which have X0 as destination register breaks DAG or MIR design, as such instruction actually is not a definitions for X0.

AArch64 has a pass that replaces defs with X0, AArch64DeadRegisterDefinitionsPass. This is how a subtract becomes a compare. So it is not unprecedented.

Interesting pass. Thank you for the reference.

It sets xzr as destination and marks it as dead. The pass is executed relatively late, just before register allocator. Probably RISC-V could use similar technique.

In D90853#2625350, @craig.topper wrote:

In order to model set rounding mode, don't we need the write_csr pseudo instruction to have an implicit def of FRMReg?

There are many system registers, putting them all to implicit definitions of write_csr does not look a flexible solution. I think about setting implicit definitions manually, using addOperand somewhere after selection.

Or we have a pseudo instruction per system register that we care about updating. It's also possible we want to add scheduling information for writing specific CSRs.

[sepavloff]

An alternative implementation of the same functionality is provided in D98936.

llvm/lib/Target/RISCV/RISCVISelLowering.h

enum NodeType : unsigned {
   // is not legal on RV64. FMV_W_X_RV64 matches the semantics of the FMV.W.X.
   // FMV_X_ANYEXTW_RV64 is similar to FMV.X.W but has an any-extended result.
   // This is a more convenient semantic for producing dagcombines that remove
   // unnecessary GPR->FPR->GPR moves.
   FMV_W_X_RV64,
   FMV_X_ANYEXTW_RV64,
   // READ_CYCLE_WIDE - A read of the 64-bit cycle CSR on a 32-bit target
   // (returns (Lo, Hi)). It takes a chain operand.
-  READ_CYCLE_WIDE
+  READ_CYCLE_WIDE,
+  // Reads value of CSR.
+  // The first operand is a chain pointer. The second specifies address of the
+  // required CSR. Two results are produced, the read value and chain pointer.
+  READ_CSR,
+  // Write value to CSR.
+  // The first operand is a chain pointer, the second specifies address of the
+  // required CSR and the third is the value to write. The result is chain
+  // pointer.
+  WRITE_CSR
 };
 }
 
 class RISCVTargetLowering : public TargetLowering {
   const RISCVSubtarget &Subtarget;
 
 public:
   explicit RISCVTargetLowering(const TargetMachine &TM,

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

const char *RISCVTargetLowering::getTargetNodeName(unsigned Opcode) const {
   case RISCVISD::REMUW:
     return "RISCVISD::REMUW";
   case RISCVISD::FMV_W_X_RV64:
     return "RISCVISD::FMV_W_X_RV64";
   case RISCVISD::FMV_X_ANYEXTW_RV64:
     return "RISCVISD::FMV_X_ANYEXTW_RV64";
   case RISCVISD::READ_CYCLE_WIDE:
     return "RISCVISD::READ_CYCLE_WIDE";
+  case RISCVISD::READ_CSR:
+    return "RISCVISD::READ_CSR";
+  case RISCVISD::WRITE_CSR:
+    return "RISCVISD::WRITE_CSR";
   }
   return nullptr;
 }
 
 /// getConstraintType - Given a constraint letter, return the type of
 /// constraint it is for this target.
 RISCVTargetLowering::ConstraintType
 RISCVTargetLowering::getConstraintType(StringRef Constraint) const {

llvm/lib/Target/RISCV/RISCVInstrInfo.td

 def SDT_CallSeqEnd   : SDCallSeqEnd<[SDTCisVT<0, i32>,
                                      SDTCisVT<1, i32>]>;
 
 // Target-dependent type requirements.
 def SDT_RISCVCall     : SDTypeProfile<0, -1, [SDTCisVT<0, XLenVT>]>;
 def SDT_RISCVSelectCC : SDTypeProfile<1, 5, [SDTCisSameAs<1, 2>,
                                              SDTCisSameAs<0, 4>,
                                              SDTCisSameAs<4, 5>]>;
+def SDT_ReadCSR       : SDTypeProfile<1, 1, [SDTCisInt<0>, SDTCisInt<1>]>;
+def SDT_WriteCSR      : SDTypeProfile<0, 2, [SDTCisInt<0>, SDTCisInt<1>]>;
 
 // Target-independent nodes, but with target-specific formats.
 def callseq_start : SDNode<"ISD::CALLSEQ_START", SDT_CallSeqStart,
                            [SDNPHasChain, SDNPOutGlue]>;
 def callseq_end   : SDNode<"ISD::CALLSEQ_END", SDT_CallSeqEnd,
                            [SDNPHasChain, SDNPOptInGlue, SDNPOutGlue]>;
 
 // Target-dependent nodes.

 def riscv_selectcc  : SDNode<"RISCVISD::SELECT_CC", SDT_RISCVSelectCC,
                              [SDNPInGlue]>;
 def riscv_tail      : SDNode<"RISCVISD::TAIL", SDT_RISCVCall,
                              [SDNPHasChain, SDNPOptInGlue, SDNPOutGlue,
                               SDNPVariadic]>;
 def riscv_sllw      : SDNode<"RISCVISD::SLLW", SDTIntShiftOp>;
 def riscv_sraw      : SDNode<"RISCVISD::SRAW", SDTIntShiftOp>;
 def riscv_srlw      : SDNode<"RISCVISD::SRLW", SDTIntShiftOp>;
+def riscv_read_csr  : SDNode<"RISCVISD::READ_CSR", SDT_ReadCSR,
+                             [SDNPHasChain]>;
+def riscv_write_csr : SDNode<"RISCVISD::WRITE_CSR", SDT_WriteCSR,
+                             [SDNPHasChain]>;
 
 //===----------------------------------------------------------------------===//
 // Operand and SDNode transformation definitions.
 //===----------------------------------------------------------------------===//
 
 class ImmXLenAsmOperand<string prefix, string suffix = ""> : AsmOperandClass {
   let Name = prefix # "ImmXLen" # suffix;
   let RenderMethod = "addImmOperands";
: RVInstR<funct7, funct3, OPC_OP, (outs GPR:$rd), (ins GPR:$rs1, GPR:$rs2),
               opcodestr, "$rd, $rs1, $rs2">;
 
 let hasNoSchedulingInfo = 1,
     hasSideEffects = 1, mayLoad = 0, mayStore = 0 in
 class CSR_ir<bits<3> funct3, string opcodestr>
     : RVInstI<funct3, OPC_SYSTEM, (outs GPR:$rd), (ins csr_sysreg:$imm12, GPR:$rs1),
               opcodestr, "$rd, $imm12, $rs1">, Sched<[WriteCSR, ReadCSR]>;
 
+// Variant of CSR_ir without output.
+let hasNoSchedulingInfo = 1,
+    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),
+              opcodestr, "$imm12, $rs1">, Sched<[WriteCSR, ReadCSR]> {
+  let rd = 0;
+}
+
 let hasNoSchedulingInfo = 1,
     hasSideEffects = 1, mayLoad = 0, mayStore = 0 in
 class CSR_ii<bits<3> funct3, string opcodestr>
     : RVInstI<funct3, OPC_SYSTEM, (outs GPR:$rd),
               (ins csr_sysreg:$imm12, uimm5:$rs1),
               opcodestr, "$rd, $imm12, $rs1">, Sched<[WriteCSR]>;
 
+// Variant of CSR_ii without output.
+let hasNoSchedulingInfo = 1,
+    hasSideEffects = 1, mayLoad = 0, mayStore = 0, isCodeGenOnly = 1 in
+class CSR_ii_wo<bits<3> funct3, string opcodestr>
+    : RVInstI<funct3, OPC_SYSTEM, (outs),
+              (ins csr_sysreg:$imm12, uimm5:$rs1),
+              opcodestr, "$imm12, $rs1">, Sched<[WriteCSR]> {
+  let rd = 0;
+}
+
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
 class ShiftW_ri<bit arithshift, bits<3> funct3, string opcodestr>
     : RVInstIShiftW<arithshift, funct3, OPC_OP_IMM_32, (outs GPR:$rd),
                     (ins GPR:$rs1, uimm5:$shamt), opcodestr,
                     "$rd, $rs1, $shamt">,
       Sched<[WriteShift32, ReadShift32]>;
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in

 def CSRRW : CSR_ir<0b001, "csrrw">;
 def CSRRS : CSR_ir<0b010, "csrrs">;
 def CSRRC : CSR_ir<0b011, "csrrc">;
 
 def CSRRWI : CSR_ii<0b101, "csrrwi">;
 def CSRRSI : CSR_ii<0b110, "csrrsi">;
 def CSRRCI : CSR_ii<0b111, "csrrci">;
 
+// Variants of the above without output value.
+def CSRW  : CSR_ir_wo<0b001, "csrw">;
+def CSRS  : CSR_ir_wo<0b010, "csrs">;
+def CSRC  : CSR_ir_wo<0b011, "csrc">;
+
+def CSRWI : CSR_ii_wo<0b101, "csrwi">;
+def CSRSI : CSR_ii_wo<0b110, "csrsi">;
+def CSRCI : CSR_ii_wo<0b111, "csrci">;
+
 /// RV64I instructions
 
 let Predicates = [IsRV64] in {
 def LWU   : Load_ri<0b110, "lwu">, Sched<[WriteLDWU, ReadMemBase]>;
 def LD    : Load_ri<0b011, "ld">, Sched<[WriteLDD, ReadMemBase]>;
 def SD    : Store_rri<0b011, "sd">, Sched<[WriteSTD, ReadStoreData, ReadMemBase]>;
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in

 defm : LdPat<load, LD>;
 
 /// Stores
 
 defm : StPat<truncstorei32, SW, GPR>;
 defm : StPat<store, SD, GPR>;
 } // Predicates = [IsRV64]
 
+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)>;
+
 /// readcyclecounter
 // On RV64, we can directly read the 64-bit "cycle" CSR.
 let Predicates = [IsRV64] in
 def : Pat<(readcyclecounter), (CSRRS CYCLE.Encoding, X0)>;
 // On RV32, ReadCycleWide will be expanded to the suggested loop reading both
 // halves of the 64-bit "cycle" CSR.
 let Predicates = [IsRV32], usesCustomInserter = 1, hasSideEffects = 0,
 mayLoad = 0, mayStore = 0, hasNoSchedulingInfo = 1 in