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

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

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

612

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

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.

lib/Target/RISCV/RISCVInstrInfo.td

 include "RISCVInstrInfoA.td"
 include "RISCVInstrInfoF.td"
 include "RISCVInstrInfoD.td"
+
+//===----------------------------------------------------------------------===//
+// Privileged instructions
+//===----------------------------------------------------------------------===//
+
+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, "">;
+
+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;
+}
+}
+
+def WFI : Priv<"wfi", 0b0001000> {
+  let rd = 0;
+  let rs1 = 0;
+  let rs2 = 0b00101;
+}
+
+let hasSideEffects = 1, mayLoad = 0, mayStore = 0 in
+def SFENCEVMA : RVInstR<0b0001001, 0b000, OPC_SYSTEM, (outs),
+                        (ins GPR:$rs1, GPR:$rs2),
+                        "sfence.vma", "$rs1, $rs2"> {
+  let rd = 0;
+}

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

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