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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

I can't help but feel the assembly syntax for the register pair instructions shouldn't include both registers (perhaps in curly braces). The implicit use of the other register when reading the source is rather ugly, and particularly hard to remember when the RV64 versions *don't* do that.



================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:442
+
+    if (STI.getFeatureBits()[RISCV::FeatureExtZpsfoperand] &&
+        !STI.getFeatureBits()[RISCV::Feature64Bit]) {
----------------
The table is called RISCV32POnly but you're checking for Zpsfoperand (whatever that mouthful of an extension is). Which is it?


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:186-188
+                       [FeatureExtZpsfoperand,
+                        FeatureExtZpn,
+                        FeatureExtZprvsfextra]>;
----------------
These aren't correct? RV64 doesn't have Zpsfoperand and RV32 doesn't have Zprvsfextra.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:145
+multiclass RVPALU64<bits<7> funct7, bits<3> funct3, string opcodestr> {
+  let DecoderNamespace = "RISCV32POnly_", Predicates = [HasStdExtZpsfoperand, IsRV32] in
+  def "32" : RVPALU64Pair<funct7, funct3, opcodestr>;
----------------
These lines look too long to me


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:176-177
+}
+
+
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
----------------
 


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:231
+def CLRS8     : RVPUnary<0b1010111, 0b00000, 0b000, "clrs8">,
+                Sched<[]>;
+def CLRS16    : RVPUnary<0b1010111, 0b01000, 0b000, "clrs16">,
----------------
Should probably have a TODO: to add scheduling information for these otherwise it'll get lost


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:216
+// Dummy zero register for pairing with X0.
+def ZERO : RISCVReg<0, "0">;
+
----------------
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)?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:226-228
+def GPRPair : RegisterClass<"RISCV", [untyped], 64, (add GPRPairs)> {
+  let Size = 64;
+}
----------------
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)


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:69
 
+def sub_lo : SubRegIndex<32>;
+def sub_hi : SubRegIndex<32, 32>;
----------------
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.


================
Comment at: llvm/test/MC/RISCV/rv64zpn-invalid.s:6-24
+# 16-bit shift
+
+# CHECK-ERROR: immediate must be an integer in the range [0, 15]
+srai16 a0, a1, 21
+
+# CHECK-ERROR: immediate must be an integer in the range [0, 15]
+srai16.u a0, a1, 21
----------------
This looks mostly the same as rv32zpn-invalid. The common tests should go in rv32zpn-invalid and have RV32 and RV64 RUN lines. Any truly RV32-only tests belong in something like rv32zpn-only-invalid. Any truly RV64-only tests can stay in rv64zpn-invalid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95588