[PATCH] D65649: [RISCV] Add MC encodings and tests of the Bit Manipulation extension

[Lewis Revill via Phabricator via llvm-commits]

lewis-revill added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:17
+
+class RVBInstShift<bits<5> funct5, bits<3> funct3, RISCVOpcode opcode,
+                   string opcodestr>
----------------
simoncook wrote:
> Probably worth a comment noting that the intent is to move these to the `InstrFormats` file once `B` has been ratified.
I'm concerned that with this definition you won't be able to enforce the immediate fitting in the field. A better approach would probably be to add an instruction format from scratch with a uimm6 shamt operand. Likewise for `RVBInstFunct5`, `RVBInstFunct6`, `RVBInstFunct7`, with the appropriate operand types for however many bits the shamt operand requires. Invalid immediate tests should also then be added.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:67
+
+class RVBInstR4Reg<bits<2> funct2, bits<3> funct3, RISCVOpcode opcode,
+                   string opcodestr, string argstr>
----------------
Is there an explanation for why this instruction is not re-using `RVInstR4` from `RISCVInstrFormats.td`?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:88
+    : RVInst<(outs GPR:$rd), (ins GPR:$rs1, GPR:$rs3, uimm5:$imm),
+             "fsriw", "$rd, $rs1, $rs3, $imm", [], InstFormatR4> {
+  bits<5> rs3;
----------------
Slight typo here, this instruction format is for more than one instruction so should use `opcodestr` rather than the hard coded `"fsriw"`.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:107
+    : RVInst<(outs GPR:$rd), (ins GPR:$rs1, GPR:$rs3, uimm5:$imm),
+             "fsriw", "$rd, $rs1, $rs3, $imm", [], InstFormatR4> {
+  bits<5> rs3;
----------------
Vice-versa here, there is still an `opcodestr` passed to this instruction format. I prefer the approach of keeping the instruction format generic even if there will only ever be one `opcodestr`, and having the instruction specify it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65649