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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
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"> {
----------------
Jim wrote:
> 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.
Then don't call it RVPShiftI6


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:201
 
+def XLenI8 : ValueTypeByHwMode<[RV32, RV64],
+                               [v4i8, v8i8]>;
----------------
Jim wrote:
> 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.
XLenVEI8VT / VXLenEI8VT / XVEI8VT / PVEI8VT / VPEI8VT all come to mind, maybe others have better ideas, but I think it's important to include the fact that it's a vector in the name otherwise it just looks like a weird way to say `i8`.


================
Comment at: llvm/test/MC/RISCV/rvp/64-bit.s:24
+add64 a0, a2, a4
+# CHECK-INST: add64 a0, a2, a4
+# CHECK-ENCODING: [0x77,0x15,0xe6,0xc0]
----------------
Please put CHECK lines above not below


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

https://reviews.llvm.org/D95588