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

[eric tang via Phabricator via llvm-commits]

tangxingxin1008 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:453
+  let Inst{31-25} = funct7;
+  let Inst{24-23} = imm2;
+  let Inst{22-20} = funct3;
----------------
jrtc27 wrote:
> This doesn't make sense to me. Why do we have an operand that's always zero and used to fill in Inst bits? Surely these should be hard-coded to 0 as part of the encoding and GPRMemAtomic (which should probably be renamed to something more general... we already abuse it downstream for non-atomic instructions) used for $rs1 to get the immdiate-less memory register operand parsing?
> 
> Though in this case shouldn't it just be a funct5? And using an RVInstR with `let rs2 = funct5` (see the atomics for example)?
Good points. Thank you for your suggestions, I will change it later.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:472
+  bits<5> imm5;
+  let Inst{11-7} = imm5;
+}
----------------
jrtc27 wrote:
> Similar to loads, should just be `let rd = 0`, surely, with GPRMemAtomic for rs1?
Good point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117733