[RISCV] DAG nodes and pseudo instructions for CSR access

[sepavloff]

Summary

New custom DAG nodes were added to represent operations on CSR. These
nodes are lowered to corresponding pseudo instruction. Using the pseudo
instructions allows to specify different scheduling information for
operations on different system registers. It also make possible to
specify dependencies of instructions on specific system registers.

[sepavloff]

This is an alternative implementation of the functionality implemented in D90853.

[sepavloff]

Use simm12 for numbers of system registers

[jrtc27]

CSR addresses are uimm12s not simm12s

llvm/lib/Target/RISCV/RISCVISelLowering.h

236

Hm, modify isn't a great name when WRITE_CSR exists

[sepavloff]

Updated patch

• used uimm12 to represent system register number,
• renamed 'modify' component of new names to 'swap',
• removed prefix 'Pseudo' in new instructions.

[sepavloff]

In D98936#2637079, @jrtc27 wrote:

CSR addresses are uimm12s not simm12s

Changed to new uimm12.

llvm/lib/Target/RISCV/RISCVISelLowering.h

236

Changed to swap.

[craig.topper]

So to provide alternate scheduling information, we need scheduler predicates to inspect the operands to find the system register?

And what pass would be responsible for adding implicit def/use of the rounding mode register?

[sepavloff]

Updated patch

• Removed asm operands from Read_CSR and friends,
• Added classes for defining operations on specific system registers.

[sepavloff]

In D98936#2637803, @craig.topper wrote:

So to provide alternate scheduling information, we need scheduler predicates to inspect the operands to find the system register?

Pseudos that operate on specific registers should get separate definitions, like:

def ReadFRM : ReadSysReg<SysRegFRM, [FRM]>;
def WriteFRM : WriteSysReg<SysRegFRM, [FRM]>;
def SwapFRM : SwapSysReg<SysRegFRM, [FRM]>;

And what pass would be responsible for adding implicit def/use of the rounding mode register?

I think the implicit use/def should be a property of FP instructions, but this is a topic for discussion (https://lists.llvm.org/pipermail/llvm-dev/2021-March/149177.html).

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

155

Does this need to inherit from Operand? Or need any of the Parser/Encoder/MCOperand etc? Could it just be an ImmLeaf down with simm32?

1178

Doesn't read need side effects to say after earlier writes to it?

[jrtc27]

Are there ever any cases where you _wouldn't_ want a CSR-specific pseudo in order to have control over the scheduling of it specifically? This feels a bit like a middle-ground that's the worst of both worlds to me.

[sepavloff]

Updated patch

• uimm12 is now only PatLeaf, not Operand.
• set hasSideEffects for Read_CSR.

[sepavloff]

In D98936#2642039, @jrtc27 wrote:

Are there ever any cases where you _wouldn't_ want a CSR-specific pseudo in order to have control over the scheduling of it specifically? This feels a bit like a middle-ground that's the worst of both worlds to me.

There may be cases when using registers to express dependencies is not practical. For hypothetical example, if write to some system register results in cache invalidation or in memory translation change, it is easier to model such dependency as a side effect.

llvm/lib/Target/RISCV/RISCVInstrInfo.td

155

Right now it may be ImmLeaf only. Changed the implementation properly.

1178

Indeed. Set hasSideEffects to true.

[sepavloff]

Ping.

[asb]

In D98936#2642039, @jrtc27 wrote:

Are there ever any cases where you _wouldn't_ want a CSR-specific pseudo in order to have control over the scheduling of it specifically? This feels a bit like a middle-ground that's the worst of both worlds to me.

Hi @jrtc27 - could you please elaborate a little on the concern that the approach in this path might be the worst of both worlds? I'm not sure I fully follow. Thanks.

[jrtc27]

In D98936#2661228, @asb wrote:

In D98936#2642039, @jrtc27 wrote:

Are there ever any cases where you _wouldn't_ want a CSR-specific pseudo in order to have control over the scheduling of it specifically? This feels a bit like a middle-ground that's the worst of both worlds to me.

Hi @jrtc27 - could you please elaborate a little on the concern that the approach in this path might be the worst of both worlds? I'm not sure I fully follow. Thanks.

If you want to have different scheduling for different CSRs, do you not need per-CSR pseudos in order to express that? This diff is ostensibly to allow for that in future, but is at a much coarser read/write/swap granularity, so doesn't really get you much over and above just scheduling CSRRW itself as a whole, just adds more complexity for little gain. IMO any CSRs we need scheduling info for should just get their own dedicated read/write/swap pseudos as and when they're needed.

[sepavloff]

In D98936#2661232, @jrtc27 wrote:

In D98936#2661228, @asb wrote:

In D98936#2642039, @jrtc27 wrote:

Are there ever any cases where you _wouldn't_ want a CSR-specific pseudo in order to have control over the scheduling of it specifically? This feels a bit like a middle-ground that's the worst of both worlds to me.

Hi @jrtc27 - could you please elaborate a little on the concern that the approach in this path might be the worst of both worlds? I'm not sure I fully follow. Thanks.

If you want to have different scheduling for different CSRs, do you not need per-CSR pseudos in order to express that? This diff is ostensibly to allow for that in future, but is at a much coarser read/write/swap granularity, so doesn't really get you much over and above just scheduling CSRRW itself as a whole, just adds more complexity for little gain. IMO any CSRs we need scheduling info for should just get their own dedicated read/write/swap pseudos as and when they're needed.

The patch D99083 demonstrates the solution for FP state/control resisters. Every register and every access get separate pseudos, each of which can have their own scheduling properties.

[jrtc27]

In D98936#2661504, @sepavloff wrote:

In D98936#2661232, @jrtc27 wrote:

In D98936#2661228, @asb wrote:

In D98936#2642039, @jrtc27 wrote:

Are there ever any cases where you _wouldn't_ want a CSR-specific pseudo in order to have control over the scheduling of it specifically? This feels a bit like a middle-ground that's the worst of both worlds to me.

Hi @jrtc27 - could you please elaborate a little on the concern that the approach in this path might be the worst of both worlds? I'm not sure I fully follow. Thanks.

If you want to have different scheduling for different CSRs, do you not need per-CSR pseudos in order to express that? This diff is ostensibly to allow for that in future, but is at a much coarser read/write/swap granularity, so doesn't really get you much over and above just scheduling CSRRW itself as a whole, just adds more complexity for little gain. IMO any CSRs we need scheduling info for should just get their own dedicated read/write/swap pseudos as and when they're needed.

The patch D99083 demonstrates the solution for FP state/control resisters. Every register and every access get separate pseudos, each of which can have their own scheduling properties.

That looks like the kind of thing I'm imagining. So can we remove the Read/Write/Swap_CSR defs from this diff and just keep the Read/Write/SwapSysReg classes for use with such pseudos?

[sepavloff]

In D98936#2661518, @jrtc27 wrote:

In D98936#2661504, @sepavloff wrote:

In D98936#2661232, @jrtc27 wrote:

In D98936#2661228, @asb wrote:

In D98936#2642039, @jrtc27 wrote:

Are there ever any cases where you _wouldn't_ want a CSR-specific pseudo in order to have control over the scheduling of it specifically? This feels a bit like a middle-ground that's the worst of both worlds to me.

Hi @jrtc27 - could you please elaborate a little on the concern that the approach in this path might be the worst of both worlds? I'm not sure I fully follow. Thanks.

If you want to have different scheduling for different CSRs, do you not need per-CSR pseudos in order to express that? This diff is ostensibly to allow for that in future, but is at a much coarser read/write/swap granularity, so doesn't really get you much over and above just scheduling CSRRW itself as a whole, just adds more complexity for little gain. IMO any CSRs we need scheduling info for should just get their own dedicated read/write/swap pseudos as and when they're needed.

The patch D99083 demonstrates the solution for FP state/control resisters. Every register and every access get separate pseudos, each of which can have their own scheduling properties.

That looks like the kind of thing I'm imagining. So can we remove the Read/Write/Swap_CSR defs from this diff and just keep the Read/Write/SwapSysReg classes for use with such pseudos?

No problem. I just thought that putting Read/Write/Swap_CSR defs (which are general purpose definitions) into a patch specific for FP support is not quite logical. But if this is OK, I'll move them.

[jrtc27]

In D98936#2661530, @sepavloff wrote:

In D98936#2661518, @jrtc27 wrote:

In D98936#2661504, @sepavloff wrote:

In D98936#2661232, @jrtc27 wrote:

In D98936#2661228, @asb wrote:

In D98936#2642039, @jrtc27 wrote:

Are there ever any cases where you _wouldn't_ want a CSR-specific pseudo in order to have control over the scheduling of it specifically? This feels a bit like a middle-ground that's the worst of both worlds to me.

Hi @jrtc27 - could you please elaborate a little on the concern that the approach in this path might be the worst of both worlds? I'm not sure I fully follow. Thanks.

If you want to have different scheduling for different CSRs, do you not need per-CSR pseudos in order to express that? This diff is ostensibly to allow for that in future, but is at a much coarser read/write/swap granularity, so doesn't really get you much over and above just scheduling CSRRW itself as a whole, just adds more complexity for little gain. IMO any CSRs we need scheduling info for should just get their own dedicated read/write/swap pseudos as and when they're needed.

The patch D99083 demonstrates the solution for FP state/control resisters. Every register and every access get separate pseudos, each of which can have their own scheduling properties.

That looks like the kind of thing I'm imagining. So can we remove the Read/Write/Swap_CSR defs from this diff and just keep the Read/Write/SwapSysReg classes for use with such pseudos?

No problem. I just thought that putting Read/Write/Swap_CSR defs (which are general purpose definitions) into a patch specific for FP support is not quite logical. But if this is OK, I'll move them.

I meant not having those defs at all, I don't see what they're useful for and are the pseudos I was taking issue with in my earlier comments.

[sepavloff]

Removed instructions Read_CSR, Write_CSR and Swap_CSR

[evandro]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1182

Methinks that hasSideEffects = 0 is fine for reading CSRs, but not sure that there's some weird register out there that changes state after being read.

1190

This should definitely be hasSideEffects = 1.

1198

As should this be hasSideEffects = 1 too.

[sepavloff]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1182

You are right. It is fairly possible and the previous version of this patch contained support for such general case (Read_CSR in https://reviews.llvm.org/D98936?vs=on&id=332569#toc). This pseudo is proposed for a particular case, which can be represented as a reading register. The concrete example is reading floating point control and state registers.

1190

Again, it is true in general case (see the removed pseudo Write_CSR in the previous version). This pseudo is for the special case, when write to system register has no effects other than changing content of the corresponding register. Floating point environment register behave exactly in this way.

1198

This is true for the removed Swap_CSR from the previous version. Maybe we need to put them back.

[sepavloff]

Rebased and added variants with immediate

[sepavloff]

Any feedback?

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

31

I think this should be SDT_RISCVReadCSR to following the convention established by the others.

1181

Should this be CSRRS to match the CSRR pseudo instruction definiton?

[sepavloff]

Addressed reviewer's notes and rebased

[sepavloff]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

31

Renamed.

1181

Changed the expansion.

[craig.topper]

LGTM

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

if (!isImm())
       return false;
     int64_t Imm;
     RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
     bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
     return IsConstantImm && isShiftedUInt<8, 2>(Imm) && (Imm != 0) &&
            VK == RISCVMCExpr::VK_RISCV_None;
   }
 
+  bool isUImm12() const {
+    if (!isImm())
+      return false;
+    int64_t Imm;
+    RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
+    bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
+    return IsConstantImm && isUInt<12>(Imm) && VK == RISCVMCExpr::VK_RISCV_None;
+  }
+
   bool isSImm12() const {
     RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
     int64_t Imm;
     bool IsValid;
     if (!isImm())
       return false;
     bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
     if (!IsConstantImm)

llvm/lib/Target/RISCV/RISCVISelLowering.h

enum NodeType : unsigned {
   VRGATHER_VX_VL,
   VRGATHER_VV_VL,
   VRGATHEREI16_VV_VL,
 
   // Vector sign/zero extend with additional mask & VL operands.
   VSEXT_VL,
   VZEXT_VL,
 
+  // 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 the new 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 the new
+  // chain pointer.
+  WRITE_CSR,
+  // Read-Modify-Write value of CSR.
+  // The first operand is a chain pointer, the second specifies address of the
+  // required CSR and the third is the value to write. Two results are produced,
+  // the value read before the modification and the new chain pointer.
+  SWAP_CSR,
+
   // Memory opcodes start here.
   VLE_VL = ISD::FIRST_TARGET_MEMORY_OPCODE,
   VSE_VL,
 
   // WARNING: Do not add anything in the end unless you want the node to
   // have memop! In fact, starting from FIRST_TARGET_MEMORY_OPCODE all
   // opcodes will be thought as target memory ops!
 };

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

#define NODE_NAME_CASE(NODE)                                                   \
   NODE_NAME_CASE(VMSET_VL)
   NODE_NAME_CASE(VRGATHER_VX_VL)
   NODE_NAME_CASE(VRGATHER_VV_VL)
   NODE_NAME_CASE(VRGATHEREI16_VV_VL)
   NODE_NAME_CASE(VSEXT_VL)
   NODE_NAME_CASE(VZEXT_VL)
   NODE_NAME_CASE(VLE_VL)
   NODE_NAME_CASE(VSE_VL)
+  NODE_NAME_CASE(READ_CSR)
+  NODE_NAME_CASE(WRITE_CSR)
+  NODE_NAME_CASE(SWAP_CSR)
   }
   // clang-format on
   return nullptr;
 #undef NODE_NAME_CASE
 }
 
 /// getConstraintType - Given a constraint letter, return the type of
 /// constraint it is for this target.

llvm/lib/Target/RISCV/RISCVInstrInfo.td

 // 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_RISCVBrCC : SDTypeProfile<0, 4, [SDTCisSameAs<0, 1>,
                                          SDTCisVT<2, OtherVT>,
                                          SDTCisVT<3, OtherVT>]>;
+def SDT_ReadCSR  : SDTypeProfile<1, 1, [SDTCisInt<0>, SDTCisInt<1>]>;
+def SDT_WriteCSR : SDTypeProfile<0, 2, [SDTCisInt<0>, SDTCisInt<1>]>;
+def SDT_SwapCSR  : SDTypeProfile<1, 2, [SDTCisInt<0>, SDTCisInt<1>, SDTCisInt<2>]>;
 def SDT_RISCVReadCycleWide : SDTypeProfile<2, 0, [SDTCisVT<0, i32>,
                                                   SDTCisVT<1, i32>]>;
 def SDT_RISCVIntBinOpW : SDTypeProfile<1, 2, [
   SDTCisSameAs<0, 1>, SDTCisSameAs<0, 2>, SDTCisVT<0, i64>
 ]>;
 
 // Target-independent nodes, but with target-specific formats.
 def callseq_start : SDNode<"ISD::CALLSEQ_START", SDT_CallSeqStart,

 def riscv_brcc      : SDNode<"RISCVISD::BR_CC", SDT_RISCVBrCC,
                              [SDNPHasChain]>;
 def riscv_tail      : SDNode<"RISCVISD::TAIL", SDT_RISCVCall,
                              [SDNPHasChain, SDNPOptInGlue, SDNPOutGlue,
                               SDNPVariadic]>;
 def riscv_sllw      : SDNode<"RISCVISD::SLLW", SDT_RISCVIntBinOpW>;
 def riscv_sraw      : SDNode<"RISCVISD::SRAW", SDT_RISCVIntBinOpW>;
 def riscv_srlw      : SDNode<"RISCVISD::SRLW", SDT_RISCVIntBinOpW>;
+def riscv_read_csr  : SDNode<"RISCVISD::READ_CSR", SDT_ReadCSR,
+                             [SDNPHasChain]>;
+def riscv_write_csr : SDNode<"RISCVISD::WRITE_CSR", SDT_WriteCSR,
+                             [SDNPHasChain]>;
+def riscv_swap_csr  : SDNode<"RISCVISD::SWAP_CSR", SDT_SwapCSR,
+                             [SDNPHasChain]>;
 
 def riscv_read_cycle_wide : SDNode<"RISCVISD::READ_CYCLE_WIDE",
                                    SDT_RISCVReadCycleWide,
                                    [SDNPHasChain, SDNPSideEffect]>;
 
 //===----------------------------------------------------------------------===//
 // Operand and SDNode transformation definitions.
 //===----------------------------------------------------------------------===//

 
 def uimm5 : Operand<XLenVT>, ImmLeaf<XLenVT, [{return isUInt<5>(Imm);}]> {
   let ParserMatchClass = UImmAsmOperand<5>;
   let DecoderMethod = "decodeUImmOperand<5>";
   let OperandType = "OPERAND_UIMM5";
   let OperandNamespace = "RISCVOp";
 }
 
+def uimm12 : Operand<XLenVT>, ImmLeaf<XLenVT, [{return isUInt<12>(Imm);}]> {
+  let ParserMatchClass = UImmAsmOperand<12>;
+  let EncoderMethod = "getImmOpValue";
+  let DecoderMethod = "decodeUImmOperand<12>";
+  let MCOperandPredicate = [{
+    int64_t Imm;
+    if (MCOp.evaluateAsConstantImm(Imm))
+      return isUInt<12>(Imm);
+    return MCOp.isBareSymbolRef();
+  }];
+  let OperandType = "OPERAND_UIMM12";
+  let OperandNamespace = "RISCVOp";
+}
+
 def simm12 : Operand<XLenVT>, ImmLeaf<XLenVT, [{return isInt<12>(Imm);}]> {
   let ParserMatchClass = SImmAsmOperand<12>;
   let EncoderMethod = "getImmOpValue";
   let DecoderMethod = "decodeSImmOperand<12>";
   let MCOperandPredicate = [{
     int64_t Imm;
     if (MCOp.evaluateAsConstantImm(Imm))
       return isInt<12>(Imm);

 def : Pat<(atomic_fence (XLenVT 7), (timm)), (FENCE 0b11, 0b11)>;
 
 // Lowering for atomic load and store is defined in RISCVInstrInfoA.td.
 // Although these are lowered to fence+load/store instructions defined in the
 // base RV32I/RV64I ISA, this lowering is only used when the A extension is
 // present. This is necessary as it isn't valid to mix __atomic_* libcalls
 // with inline atomic operations for the same object.
 
+/// Access to system registers
+
+// General operations. A system register is represented in them by its number.
+// Dependencies between such operation and other instructions are expressed via
+// side effect.
+
+let hasSideEffects = 0, isCodeGenOnly = 1 in
+def Read_CSR : Pseudo<(outs GPR:$rd), (ins csr_sysreg:$reg),
+                      [(set GPR:$rd, (riscv_read_csr uimm12:$reg))]>,
+              PseudoInstExpansion<(CSRRC GPR:$rd, csr_sysreg:$reg, X0)>;
+
+let hasSideEffects = 1, isCodeGenOnly = 1 in
+def Write_CSR : Pseudo<(outs), (ins csr_sysreg:$reg, GPR:$val),
+                       [(riscv_write_csr uimm12:$reg, GPR:$val)]>,
+               PseudoInstExpansion<(CSRRW X0, csr_sysreg:$reg, GPR:$val)>;
+
+let hasSideEffects = 1, isCodeGenOnly = 1 in
+def Swap_CSR : Pseudo<(outs GPR:$rd), (ins csr_sysreg:$reg, GPR:$val),
+                      [(set GPR:$rd, (riscv_swap_csr uimm12:$reg, GPR:$val))]>,
+              PseudoInstExpansion<(CSRRW GPR:$rd, csr_sysreg:$reg, GPR:$val)>;
+
+// Helpers for defining specific operations. They are defined for each system
+// register separately. Side effect is not used because dependencies are
+// expressed via use-def properties.
+
+class ReadSysReg<SysReg SR, list<Register> Regs>
+  : Pseudo<(outs GPR:$rd), (ins),
+           [(set GPR:$rd, (riscv_read_csr (XLenVT SR.Encoding)))]>,
+    PseudoInstExpansion<(CSRRC GPR:$rd, SR.Encoding, X0)> {
+  let hasSideEffects = 0;
+  let Uses = Regs;
+}
+
+class WriteSysReg<SysReg SR, list<Register> Regs>
+  : Pseudo<(outs), (ins GPR:$val),
+           [(riscv_write_csr (XLenVT SR.Encoding), GPR:$val)]>,
+    PseudoInstExpansion<(CSRRW X0, SR.Encoding, GPR:$val)> {
+  let hasSideEffects = 0;
+  let Defs = Regs;
+}
+
+class SwapSysReg<SysReg SR, list<Register> Regs>
+  : Pseudo<(outs GPR:$rd), (ins GPR:$val),
+           [(set GPR:$rd, (riscv_swap_csr (XLenVT SR.Encoding), GPR:$val))]>,
+    PseudoInstExpansion<(CSRRW GPR:$rd, SR.Encoding, GPR:$val)> {
+  let hasSideEffects = 0;
+  let Uses = Regs;
+  let Defs = Regs;
+}
+
 /// Other pseudo-instructions
 
 // Pessimistically assume the stack pointer will be clobbered
 let Defs = [X2], Uses = [X2] in {
 def ADJCALLSTACKDOWN : Pseudo<(outs), (ins i32imm:$amt1, i32imm:$amt2),
                               [(callseq_start timm:$amt1, timm:$amt2)]>;
 def ADJCALLSTACKUP   : Pseudo<(outs), (ins i32imm:$amt1, i32imm:$amt2),
                               [(callseq_end timm:$amt1, timm:$amt2)]>;