[PATCH] D40383: Add RISCV privileged instructions

[Alex Bradbury via Phabricator via llvm-commits]

asb added subscribers: llvm-commits, apazos, sabuasal, mgrang, shiva0217.
asb added a comment.

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 <http://lists.llvm.org/pipermail/cfe-dev/2017-November/056032.html> 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



================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:583
+let hasSideEffects = 1, mayLoad = 0, mayStore = 0 in
+class Prv_ri<string opcodestr>
+    : RVInstI<0b000, OPC_SYSTEM, (outs), (ins), opcodestr, "">;
----------------
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.


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:611
+let hasSideEffects = 1, mayLoad = 0, mayStore = 0 in
+def SFENCEVM : RVInstR<0b0001001, 0b000, OPC_SYSTEM, (outs),
+                  (ins GPR:$rs1, GPR:$rs2),
----------------
SFENCE_VMA would match the naming convention of other instructions with a . in the name.


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:612
+def SFENCEVM : RVInstR<0b0001001, 0b000, OPC_SYSTEM, (outs),
+                  (ins GPR:$rs1, GPR:$rs2),
+                  "sfence.vma", "$rs1, $rs2"> {
----------------
These two lines should be indented to aligned with the 0 in `RVInstR<0...`


================
Comment at: test/MC/RISCV/priv.s:10
+
+  # CHECK-INST: ecall
+# CHECK: encoding: [0x73,0x00,0x00,0x00]
----------------
Unwanted whitespace, also this patch doesn't add ecall.


================
Comment at: test/MC/RISCV/priv.s:14
+
+  # CHECK-INST: ebreak
+# CHECK: encoding: [0x73,0x00,0x10,0x00]
----------------
Unwanted whitespace, also this patch doesn't add ebreak.


Repository:
  rL LLVM

https://reviews.llvm.org/D40383