[PATCH] D117733: [RISCV] Update Privileged spec to version-20211203: Support Hypervisor Extention

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.
Herald added a subscriber: pcwang-thead.

Thanks for the patch. Alongside the inline comments, two minor suggestions:

- There's no need to split it out now I don't think, but the GPRAtomicMemOp->GPRMemZeroOffset is a change that could have been split out to a separate commit and reviewed separately.
- We don't do enough testing for this in general I don't think, but it would be good to add tests that attempting to use the RV64-only hypervisor instructions on RV32 produces an error.



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:106
 
+// A parse method for (${gpr}) or 0(${gpr}), where the 0 is be silently ignored.
+// Used for GNU as Compatibility.
----------------
I know this comment is copied, but as you're touching it perhaps reword to "A parse method for (${gpr}) or 0(${gpr}), where the 0 will be silently ignored." I'd drop the "Used for GNU as compatibility" bit as I don't think it's adding much information.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:743
 
+def HLV_B : HLoad_r<0b0110000, 0b00000, "hlv.b">, Sched<[]>;
+def HLV_BU : HLoad_r<0b0110000, 0b00001, "hlv.bu">, Sched<[]>;
----------------
For a block of instructions like this, we typically place the `:` at the some column on every line, meaning the right hand side is aligned (and so easy to scan to check the integer parameters).


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:751
+let Predicates = [IsRV64] in {
+def HLV_WU : HLoad_r<0b0110100, 0b00001, "hlv.wu">, Sched<[]>;
+def HLV_D : HLoad_r<0b0110110, 0b00000, "hlv.d">, Sched<[]>;
----------------
The general preference is to match the order of instructions in the instruction listing table (table 9.1 in the ratified privileged spec update). This would put the RV64 only instructions in a group after the load+store instructions that are available in both RV32 and RV64.


================
Comment at: llvm/test/MC/RISCV/priv-invalid.s:1
-# RUN: not llvm-mc -triple riscv32 < %s 2>&1 | FileCheck %s
+# RUN: not llvm-mc -triple riscv64 < %s 2>&1 | FileCheck %s
 
----------------
If anything, it would be better to add a second RUN line for riscv64 alongside the existing riscv32. You could then have priv-rv64-invalid.s for the RV64 only instructions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117733/new/

https://reviews.llvm.org/D117733