[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/AsmParser/RISCVAsmParser.cpp:949
 
+  bool IsRegGPR = RISCVMCRegisterClasses[RISCV::GPRRegClassID].contains(Reg);
+
----------------
Don't need a blank line above, and I'd put GPR stuff before FPR stuff.


================
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;
----------------
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?


================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:403
+            dbgs()
+            << "Trying RISCV32POnly_32 table (SIMD 32-bit Instruction):\n");
+        // Calling the auto-generated decoder function.
----------------
You should not need both namespaces, only one of them. I'd put the weirdo register pair instructions in a namespace and leave the normal non-pair RV64 versions in the standard namespace.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrFormats.td:130
 def OPC_OP_FP     : RISCVOpcode<0b1010011>;
+def OPC_OP_P      : RISCVOpcode<0b1111111>;
 def OPC_OP_V      : RISCVOpcode<0b1010111>;
----------------
As I said in the other review, this goes against what the RISCV spec said it would do, as it's using an encoding it proposed reserved for >32-bit instructions. I'm not a fan of merging this until it's confirmed that this is the way things are definitely going to go, as it may affect downstreams that use the >32-bit encoding space to steer clear of conflicts (like hwacha did), but maybe there aren't any for us?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:52
+    if (STI.getTargetTriple().isArch64Bit())
+      return  isUInt<3>(Imm);
+    return isUInt<2>(Imm);
----------------
Weird spacing


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:74
+
+let Constraints = "$rs3 = $rd", hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+class RVPTernary<bits<7> funct7, bits<3> funct3, string opcodestr>
----------------
Style has been to put such constraints in the body rather than a let (see A and C).


================
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"> {
----------------
That doesn't look like a uimm6 to me


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:69
 
+def sub_lo : SubRegIndex<32>;
+def sub_hi : SubRegIndex<32, 32>;
----------------
This assumes RV32, and is not clear it applies to register pairs


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


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedRocket.td:19
   let MispredictPenalty = 3;
-  let UnsupportedFeatures = [HasStdExtV, HasStdExtZvamo, HasStdExtZvlsseg];
+  let UnsupportedFeatures = [HasStdExtV, HasStdExtZvamo, HasStdExtZvlsseg, HasStdExtP];
 }
----------------
I've never understood why this isn't a SupportedFeatures field given processors don't grow new features but compilers do...


================
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
----------------
You don't need rvXXp- prefixes on your files if they're inside rvp. Just use rvXX- if you need to specify XLEN.


================
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
----------------
This is almost the same file as rv32p-invalid.s. Merge the common parts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95588