[PATCH] D153748: [RISCV] Add support for XCValu extension in CV32E40P

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:772
+    : SubtargetFeature<"xcvalu", "HasVendorXCValu", "true",
+                       "'XCValu' (ALU Operations)">;
+def HasVendorXCValu : Predicate<"Subtarget->hasVendorXCValu()">,
----------------
Can you put `CORE-V` in this string too


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td:233
+                  Sched<[]>;
+  def CV_SLET   : CVInstAlu_rr<0b0101001, 0b011, (outs GPR:$rd), (ins GPR:$rs1, GPR:$rs2),
+                               "cv.slet", "$rd, $rs1, $rs2">,
----------------
I would have expected cv.sle and cv.sleu for these mnemonics. The `t` feels out of place. In the base isa for slt/sltu it's part of "less than". Is the instruction name here "set less equal than" instead "set less than or equal"?

The vector spec uses "sle" and "sleu"

Is the spec frozen?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td:260
+                  Sched<[]>;
+  def CV_EXTBZ  : CVInstAlu_r<0b0110011, 0b011, (outs GPR:$rd), (ins GPR:$rs1),
+                              "cv.extbz", "$rd, $rs1">,
----------------
I'm surprised this instruction exists. Isn't this `andi rs1, 255`?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td:305
+  hasSideEffects = 0, mayLoad = 0, mayStore = 0, Opcode = OPC_CUSTOM_1.Value, Constraints = "$rd = $rd_wb" in {
+  def CV_ADDNR   : CVInstAlu_rr<0b1000000, 0b011, (outs GPR:$rd_wb), (ins GPR:$rd, GPR:$rs1, GPR:$rs2),
+                                "cv.addnr", "$rd, $rs1, $rs2">,
----------------
Wrap to 80 columns. by putting `(ins GPR:$rd, GPR:$rs1, GPR:$rs2),` on a new line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153748