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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
