[PATCH] D95588: [RISCV] Implement the MC layer support of P extension

[Jim Lin via Phabricator via llvm-commits]

Jim marked 4 inline comments as done.
Jim added inline comments.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:966
+  // GPRPair is specified by first register of even/odd pair of registers.
+  if (IsRegGPR && !isRV64() && Kind == MCK_GPRPair && convertGPRToGPRPair(Op))
+    return Match_Success;
----------------
jrtc27 wrote:
> I do not like the side-effecting function. Please make it look like the FPR code, which probably means splitting out the "is GPR pair" part from convertGPRToGPRPair (i.e. checking if it's an even register number). Also, seeing MCK_GPRPair for non-RV32 should always be an error (assertion failure), surely, and not just something to ignore?
Inst with MCK_GPRPair version is still trying to match operand for non-RV32. It would be a error due to missing features later.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:117
+    : RVInstI<funct3, OPC_OP_P, (outs GPRP:$rd),
+              (ins GPRP:$rs1, uimmlog2xlen:$shamt),
+              opcodestr, "$rd, $rs1, $shamt"> {
----------------
jrtc27 wrote:
> That doesn't look like a uimm6 to me
Do you mean that is not the same as RVPShiftI5 with uimm5. It is not uimm6 here.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:201
 
+def XLenI8 : ValueTypeByHwMode<[RV32, RV64],
+                               [v4i8, v8i8]>;
----------------
jrtc27 wrote:
> Needs a VT suffix, but it's not clear from the name what exactly this is
Add suffix VT. Do you have any suggestion for this? Thanks.


================
Comment at: llvm/test/MC/RISCV/rvp/rv32p-invalid.s:1
+# RUN: not llvm-mc -triple riscv32 -mattr=+experimental-p < %s 2>&1 \
+# RUN:        | FileCheck %s --check-prefix=CHECK-ERROR
----------------
jrtc27 wrote:
> You don't need rvXXp- prefixes on your files if they're inside rvp. Just use rvXX- if you need to specify XLEN.
The range for immediate is difference between RV32 and RV64.


================
Comment at: llvm/test/MC/RISCV/rvp/rv64p-invalid.s:1
+# RUN: not llvm-mc -triple riscv64 -mattr=+experimental-p < %s 2>&1 \
+# RUN:        | FileCheck %s --check-prefix=CHECK-ERROR
----------------
jrtc27 wrote:
> This is almost the same file as rv32p-invalid.s. Merge the common parts.
The immediate range is difference between RV32 and RV64.


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

https://reviews.llvm.org/D95588