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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:442
+
+    if (STI.getFeatureBits()[RISCV::FeatureExtZpsfoperand] &&
+        !STI.getFeatureBits()[RISCV::Feature64Bit]) {
----------------
Jim wrote:
> jrtc27 wrote:
> > The table is called RISCV32POnly but you're checking for Zpsfoperand (whatever that mouthful of an extension is). Which is it?
> Rename RISCV32POnly to RISCV32Zpsfoperand. 
> This is for the instruction with even/odd paired-register operands on RV32. 
> 
> In RISCV32Zpsfoperand, two kinds of instruction defined for the same instruction in spec, one for RV32 with even/odd paired-register operands defined in RISCV32Zpsfoperand table , the other one for RV64 with normal GPR operands.
> 
> 
Is there a reason why this needs to be limited to Zpsfoperand? We don't have separate namespaces for every extension, we only add them when there are conflicts (with the "more common" extension being in the default namespace). I'd expect RISCV32Only_ with just a Feature64Bit check, like we do for some RV32C instructions, to be sufficient.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:10
+/// This file describes the RISC-V instructions from the standard 'P' SIMD
+/// extension, version 0.96.
+/// This version is still experimental as the 'P' extension hasn't been
----------------
Isn't it 0.9.6 (or 0.9.7 now)? Not that that's a legal extension version number...


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:147
+      Predicates = [HasStdExtZpsfoperand, IsRV32] in
+  def "32" : RVPALU64Pair<funct7, funct3, opcodestr>;
+  let Predicates = [HasStdExtZpsfoperand, IsRV64] in
----------------
FOO32 vs FOO64 intuitively implies something about the width of operands to me, not what ISA they're for. For B the instruction definitions use _RV32 and _RV64 suffices.

Especially since the instructions are already called things like ADD64, this will generate nonsense ADD6432 and ADD6464 names...


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:169
+    : RVInstR<funct7, funct3, OPC_OP_P, (outs GPR32PairOp:$rd),
+              (ins GPR:$rs1, GPR:$rs2),
+              opcodestr, "$rd, $rs1, $rs2">;
----------------
This, RVPSMAL64Pair and RVPALU64Pair are all just RVPBinary with different numbers of the operands changed to GPR32PairOp, I think this would be better with default (to GPR) template arguments added to RVPBinary for rd/rs1/rs2 rather than having three almost-identical copies of the same thing under varying names


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:181
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+class RVPMA64Pair<bits<7> funct7, bits<3> funct3, string opcodestr>
+    : RVInstR<funct7, funct3, OPC_OP_P, (outs GPR32PairOp:$rd),
----------------
Similarly just make RVPTernary take template arguments for the types (in this case only one argument, for rd/rs3, needed)


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:909
+
+// kmar64 has a aliased instruction kmada32 belong to zpn sub-extension on RV64.
+let DecoderNamespace = "RISCV32Zpsfoperand_",
----------------
Having the same instruction in two different extensions under two different names is insane. Currently this implementation lets you use the "wrong" name for kmar64 with Zpn. But I would prefer the spec weren't crazy.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:219
+def GPR32Pairs : RegisterTuples<[gpr32_pair_lo, gpr32_pair_hi],
+                                [(add X0,  X2,  X4,  X6,
+                                      X8,  X10, X12, X14,
----------------
Won't the order affect register allocation? Technically not an issue for this MC patch but the correct thing should be committed so there's no churn when adding codegen.

That or don't reuse this in the register class below and instead manually list the pairs in the class.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:228
+
+def GPR32Pair : RegisterClass<"RISCV", [untyped], 64, (add GPR32Pairs)> {
+  let Size = 64;
----------------
Why is this untyped?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95588