[PATCH] D94579: [RISCV] add the MC layer support of P extension

[Jim Lin via Phabricator via llvm-commits]

Jim added a comment.

It seems lack of invalid operand handling for new added operand (uimm3, uimm4) in AsmParser/RISCVAsmParser.cpp:MatchAndEmitInstruction.



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:26
+class ALU_P_r<bits<7> funct7, bits<5> funct5, bits<3> funct3, string opcodestr>
+    : RVInstR<funct7, funct3, OPC_OP_P, (outs GPR:$rd), (ins GPR:$rs1, GPR:$rs2),
+              opcodestr, "$rd, $rs1"> {
----------------
This should be (ins GPR:$rs1)


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:28
+              opcodestr, "$rd, $rs1"> {
+    let rs2 = funct5;
+}
----------------
This should be "let Inst{24-20} = funct5"


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:154
+def KDMABT    : ALU_P_rr<0b1110001, 0b001, "kdmabt">;
+def KDMATT    : ALU_P_rr<0b1111001, 0b001, "kdmatt">;
+def KHM8      : ALU_P_rr<0b1000111, 0b000, "khm8">;
----------------
KDMABB's rd register is also a input operand. It should create another class for this kind instruction.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:507
+
+} // Predicates = [HasStdExtP, IsRV64]
----------------
Could you refer RISCVInstrInfoV.td to add comment between operand, class declaration and instruction definition.
For more readable, could you add category name before instructions.


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:56
   bool HasStdExtZvamo = false;
+  bool HasStdExtP = false;
   bool HasStdExtZfh = false;
----------------
Put it between B and V in the canonical extension order


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:123
   bool hasStdExtZvamo() const { return HasStdExtZvamo; }
+  bool hasStdExtP() const { return HasStdExtP; }
   bool hasStdExtZfh() const { return HasStdExtZfh; }
----------------
Put it between B and V in the canonical extension order


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94579