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

[Jim Lin via Phabricator via cfe-commits]

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


================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:442
+
+    if (STI.getFeatureBits()[RISCV::FeatureExtZpsfoperand] &&
+        !STI.getFeatureBits()[RISCV::Feature64Bit]) {
----------------
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.




================
Comment at: llvm/lib/Target/RISCV/RISCV.td:186-188
+                       [FeatureExtZpsfoperand,
+                        FeatureExtZpn,
+                        FeatureExtZprvsfextra]>;
----------------
jrtc27 wrote:
> These aren't correct? RV64 doesn't have Zpsfoperand and RV32 doesn't have Zprvsfextra.
RV64 has Zpsfoperand extension that just has normal GPRs as operands (RV32 has even/odd paired-register operand).

RV32 doesn't have Zprvsfextra. All of instruction in Zprvsfextra are defined in Predicates = [HasStdExtZprvsfextra, IsRV64].

If P is enabled, it means Zpn+Zpsfoperand enabled on RV32, and Zpn+Zpsfoperand+Zprvsfextra enabled on RV64.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:216
+// Dummy zero register for pairing with X0.
+def ZERO : RISCVReg<0, "0">;
+
----------------
jrtc27 wrote:
> Ew, this is a gross quirk of the register pair instructions. ZERO is not a good name for it though, that's the ABI name for x0 so already taken and is pretty confusing. I assume LLVM doesn't like it if you create a register pair that is (X0, X0)?
Rename it to REG_PAIR_WITH_X0.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:226-228
+def GPRPair : RegisterClass<"RISCV", [untyped], 64, (add GPRPairs)> {
+  let Size = 64;
+}
----------------
jrtc27 wrote:
> IMO the register class should be GPR32Pair not GPRPair unless you also make it have a sensible interpretation for RV32 (which seems like a waste of time)
Rename it to GPR32Pair.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:69
 
+def sub_lo : SubRegIndex<32>;
+def sub_hi : SubRegIndex<32, 32>;
----------------
jrtc27 wrote:
> Jim wrote:
> > Jim wrote:
> > > luismarques wrote:
> > > > jrtc27 wrote:
> > > > > This assumes RV32, and is not clear it applies to register pairs
> > > > What's the best way to address this?
> > > sub_lo and sub_hi are only used for GPRPair register class to extract a register from pair registers on RV32.
> > Do you mean that sub_lo and sub_hi only used on RV32? Does it need to rename or ..?
> Yes, these should have names that make it clear they're for each half of a 2*32-bit register pair. Otherwise it sounds like they're the 32-bit hi and lo halves of the 64-bit registers on RV64.
Rename it to gpr_pair_lo and gpr_pair_hi


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95588