[PATCH] D152915: [RISCV] Add support for XCVbitmanip extension in CV32E40P

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td:15
+                    string opcodestr, string argstr, list<dag> pattern>
+    : RVInst<outs, ins, opcodestr, argstr, pattern, InstFormatOther> {
+  bits<5> is3;
----------------
I think you can use RVInstI and a `let imm12 = {funct2, is3, is2};`


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td:34
+                   string opcodestr, string argstr, list<dag> pattern>
+    : RVInst<outs, ins, opcodestr, argstr, pattern, InstFormatOther> {
+  bits<5> rs2;
----------------
I think you can use RVInstR?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td:51
+                  string opcodestr, string argstr, list<dag> pattern>
+    : RVInst<outs, ins, opcodestr, argstr, pattern, InstFormatOther> {
+  bits<5> rs1;
----------------
I think you can use RVInstR and a `let rs2 = 0b00000`


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td:70
+class CVBitManipRR<bits<7> funct7, string opcodestr>
+  : RVInstBitManipRR<funct7, 0b011, (outs GPR:$rd), (ins GPR:$rs1, GPR:$rs2),
+                     opcodestr, "$rd, $rs1, $rs2", []>;
----------------
I think you can use RVInstR and merge the remaining fields of `RVInstBitManipRR` here?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td:85
+  def CV_BITREV : RVInstBitManipRII<0b11, 0b001, (outs GPR:$rd), 
+                                    (ins GPR:$rs1, uimm2:$is3, uimm5:$is2),
+                                    "cv.bitrev", "$rd, $rs1, $is3, $is2", []>;
----------------
Is the only difference between this and CVBitManipRII the `uimm2` for $is3? Maybe you could make that a template argument to CVBitManipRII that defaults to uimm5, but can be changed.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td:93
+  def CV_INSERT : RVInstBitManipRII<0b10, 0b000, (outs GPR:$rd_wb),
+                                (ins GPR:$rs1, uimm5:$is3, uimm5:$is2, GPR:$rd),
+                                "cv.insert", "$rd, $rs1, $is3, $is2", []>;
----------------
I think it's usual to put $rd first in ins?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td:96
+  def CV_INSERTR : RVInstBitManipRR<0b0011010, 0b011, (outs GPR:$rd_wb),
+                                (ins GPR:$rs1, GPR:$rs2, GPR:$rd),
+                                "cv.insertr", "$rd, $rs1, $rs2", []>;
----------------
Same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152915