Add RISCV privileged instructions

[dvc94ch]

Summary

Adds the instructions defined in the RISCV privileged ISA. (uret, sret, mret, wfi, sfence.vma)

[asb]

Hi David, many thanks for submitting this and welcome to the upstream LLVM community!

I'm adding the llvm-commits mailing list as a subscriber. Unfortunately this isn't done automatically and is necessary to ensure the patch is shared to that mailing list. There's a proposal to fix this annoyance.

I've left a few code review comments, all pretty minor stuff regarding naming and whitespace. Although the privileged spec only lists the I-type instruction form, it seems to me that {U,S,M}RET, WFI, and SFENCE.VMA are actually presented as R-type instructions.

Should uret, sret and mret have let isBarrier = 1, isReturn = 1, isTerminator = 1?

For anyone else reviewing this patch, these instructions are defined in the draft privileged spec: https://content.riscv.org/wp-content/uploads/2017/05/riscv-privileged-v1.10.pdf

lib/Target/RISCV/RISCVInstrInfo.td

583 ↗

(On Diff #124042)

The I use the _ri or _rr suffixes to differentiate reg-reg and reg-imm operands. As these instructions have no operands, just Prv or Priv would be the better class name.

611 ↗

(On Diff #124042)

SFENCE_VMA would match the naming convention of other instructions with a . in the name.

612 ↗

(On Diff #124042)

These two lines should be indented to aligned with the 0 in RVInstR<0...

test/MC/RISCV/priv.s

10 ↗

(On Diff #124042)

Unwanted whitespace, also this patch doesn't add ecall.

14 ↗

(On Diff #124042)

Unwanted whitespace, also this patch doesn't add ebreak.

[asb]

One more thing - priv.s would be better named priv-valid.s, and it would be great to have a priv-invalid.s alongside it that checks that obviously invalid forms are rejected (e.g. sret with a register argument, sfence.vma with too few arguments).

[dvc94ch]

Addressed all comments.

rename priv.s to priv-valid.s, added priv-invalid.s, removed ecall/ebreak test cases (where added because those instructions are explicitly mentioned in priv spec)

renamed tablegen class to Priv, added isTerminator, isBarrier, isReturn to mret, sret, uret

[asb]

Is there any reason not to include the ecall/ebreak instruction definitions in this patch? (My apologies - I should have directly asked this when commenting on the fact that ecall/ebreak tests were present but not the definitions).

For what it's worth, the R-type format is now listed in the privileged ISA description https://github.com/riscv/riscv-isa-manual/commit/9f22e9a9d7b3e3d9649fb0a9d1d0b48821037222. Switching the definitions of URET, SRET, MRET, WFI, SFENCE.VMA to use RVInstR would be slightly preferable.

lib/Target/RISCV/RISCVInstrInfo.td

586 ↗

(On Diff #124060)

Along with some of the other backends, we typically treat let Foo = .. much like a C++ namespace declaration in terms of indentation (LLVM guidelines).

See RISCVInstrInfo.td for some examples.

[dvc94ch]

• Uses RVInstR as the base class for Priv
• Fixed indentation

[asb]

Thanks! This looks good to me.

llvm/trunk/lib/Target/RISCV/RISCVInstrInfo.td

: RVInstIShiftW<arithshift, funct3, OPC_OP_IMM_32, (outs GPR:$rd),
                     (ins GPR:$rs1, uimm5:$shamt), opcodestr,
                     "$rd, $rs1, $shamt">;
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
 class ALUW_rr<bits<7> funct7, bits<3> funct3, string opcodestr>
     : RVInstR<funct7, funct3, OPC_OP_32, (outs GPR:$rd),
               (ins GPR:$rs1, GPR:$rs2), opcodestr, "$rd, $rs1, $rs2">;
 
+let hasSideEffects = 1, mayLoad = 0, mayStore = 0 in
+class Priv<string opcodestr, bits<7> funct7>
+    : RVInstR<funct7, 0b000, OPC_SYSTEM, (outs), (ins GPR:$rs1, GPR:$rs2),
+              opcodestr, "">;
+
 //===----------------------------------------------------------------------===//
 // Instructions
 //===----------------------------------------------------------------------===//
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
 def LUI : RVInstU<OPC_LUI, (outs GPR:$rd), (ins uimm20:$imm20),
                   "lui", "$rd, $imm20">;
 

 def ADDW  : ALUW_rr<0b0000000, 0b000, "addw">;
 def SUBW  : ALUW_rr<0b0100000, 0b000, "subw">;
 def SLLW  : ALUW_rr<0b0000000, 0b001, "sllw">;
 def SRLW  : ALUW_rr<0b0000000, 0b101, "srlw">;
 def SRAW  : ALUW_rr<0b0100000, 0b101, "sraw">;
 } // Predicates = [IsRV64]
 
 //===----------------------------------------------------------------------===//
+// Privileged instructions
+//===----------------------------------------------------------------------===//
+
+let isBarrier = 1, isReturn = 1, isTerminator = 1 in {
+def URET : Priv<"uret", 0b0000000> {
+  let rd = 0;
+  let rs1 = 0;
+  let rs2 = 0b00010;
+}
+
+def SRET : Priv<"sret", 0b0001000> {
+  let rd = 0;
+  let rs1 = 0;
+  let rs2 = 0b00010;
+}
+
+def MRET : Priv<"mret", 0b0011000> {
+  let rd = 0;
+  let rs1 = 0;
+  let rs2 = 0b00010;
+}
+} // isBarrier = 1, isReturn = 1, isTerminator = 1
+
+def WFI : Priv<"wfi", 0b0001000> {
+  let rd = 0;
+  let rs1 = 0;
+  let rs2 = 0b00101;
+}
+
+let hasSideEffects = 1, mayLoad = 0, mayStore = 0 in
+def SFENCE_VMA : RVInstR<0b0001001, 0b000, OPC_SYSTEM, (outs),
+                         (ins GPR:$rs1, GPR:$rs2),
+                         "sfence.vma", "$rs1, $rs2"> {
+  let rd = 0;
+}
+
+//===----------------------------------------------------------------------===//
 // Pseudo-instructions and codegen patterns
 //
 // Naming convention: For 'generic' pattern classes, we use the naming
 // convention PatTy1Ty2. For pattern classes which offer a more complex
 // expension, prefix the class name, e.g. BccPat.
 //===----------------------------------------------------------------------===//
 
 /// Generic pattern classes

llvm/trunk/test/MC/RISCV/priv-invalid.s

+# RUN: not llvm-mc -triple riscv32 < %s 2>&1 | FileCheck %s
+
+mret 0x10 # CHECK: :[[@LINE]]:6: error: invalid operand for instruction
+
+sfence.vma zero # CHECK: :[[@LINE]]:1: error: too few operands for instruction
+
+sfence.vma a0, 0x10 # CHECK: :[[@LINE]]:16: error: invalid operand for instruction

llvm/trunk/test/MC/RISCV/priv-valid.s

+# RUN: llvm-mc %s -triple=riscv32 -show-encoding \
+# RUN:     | FileCheck -check-prefixes=CHECK,CHECK-INST %s
+# RUN: llvm-mc %s -triple=riscv64 -show-encoding \
+# RUN:     | FileCheck -check-prefixes=CHECK,CHECK-INST %s
+# RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
+# RUN:     | llvm-objdump -d - | FileCheck -check-prefix=CHECK-INST %s
+# RUN: llvm-mc -filetype=obj -triple riscv64 < %s \
+# RUN:     | llvm-objdump -d - | FileCheck -check-prefix=CHECK-INST %s
+
+# CHECK-INST: uret
+# CHECK: encoding: [0x73,0x00,0x20,0x00]
+uret
+
+# CHECK-INST: sret
+# CHECK: encoding: [0x73,0x00,0x20,0x10]
+sret
+
+# CHECK-INST: mret
+# CHECK: encoding: [0x73,0x00,0x20,0x30]
+mret
+
+# CHECK-INST: wfi
+# CHECK: encoding: [0x73,0x00,0x50,0x10]
+wfi
+
+# CHECK-INST: sfence.vma zero, zero
+# CHECK: encoding: [0x73,0x00,0x00,0x12]
+sfence.vma zero, zero
+
+# CHECK-INST: sfence.vma a0, a1
+# CHECK: encoding: [0x73,0x00,0xb5,0x12]
+sfence.vma a0, a1