[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/RISCVRegisterInfo.td:219
+def GPR32Pairs : RegisterTuples<[gpr32_pair_lo, gpr32_pair_hi],
+                                [(add X0,  X2,  X4,  X6,
+                                      X8,  X10, X12, X14,
----------------
jrtc27 wrote:
> 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.
You didn't need to manually list them all here...


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:228
+
+def GPR32Pair : RegisterClass<"RISCV", [untyped], 64, (add GPR32Pairs)> {
+  let Size = 64;
----------------
Jim wrote:
> jrtc27 wrote:
> > Why is this untyped?
> GPR32Pair has untyped type. 
> In code generation, It captures some operation with i64 type supported by Zpsoperand to untyped during legalization. 
> In my mind, untyped is used to be represented special type don't need any legalization.
> I refer to ARMRegisterInfo.td that defines GPRPair with untyped.
So, presumably it can't be i64 because that would then make i64 a legal type? Maybe that's fine, maybe it's not, but alternatively it could be typed as v2i32 here (can include v4i16 and v8i8 too) and cast if/when needed. Craig maybe you have thoughts on this? But untyped always feels like laziness to me that results in TableGen being unable to help you when it comes to type checking, with rare exceptions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95588