[PATCH] D136172: [AArch64]SME2 Multi vector Sel Load and Store instructions

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

I've not reviewed the td files yet but...



================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1419-1430
+    if (Kind != k_VectorList)
+      return DiagnosticPredicateTy::NoMatch;
+    if (VectorList.Count != NumRegs)
+      return DiagnosticPredicateTy::NoMatch;
+    if (VectorList.RegisterKind != VectorKind)
+      return DiagnosticPredicateTy::NoMatch;
+    if (VectorList.NumElements != 0)
----------------
Is it not possible to reuse `isTypedVectorList` much like you did for `isTypedVectorListMultiple`?


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1800
+    case 2:
+      if ((getVectorListStart() - AArch64::Z0) < 16)
+        Inst.addOperand(MCOperand::createReg(
----------------
Do you need to be stricter here because I believe only z0-z7 is allowed here and the else block only allows z16-z23? Either that or you need some extra asserts before each of the calls to `Inst.addOperand`.

My guess is the asserts are enough because we should have already failed before getting here if the registers are invalid.


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp:1540
       // Because it is a wrap-around register.
-      Reg < getNextVectorRegister(Reg, NumRegs - 1)) {
+      Reg < getNextVectorRegister(Reg, NumRegs - 1) && Stride == 1) {
     printRegName(O, Reg);
----------------
Can this be put after `NumRegs > 1 &&` so we don't call `getNextVectorRegister` unnecessarily?


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp:1555-1556
       if (MRI.getRegClass(AArch64::ZPRRegClassID).contains(Reg) ||
+          MRI.getRegClass(AArch64::ZPR2RegClassID).contains(Reg) ||
+          MRI.getRegClass(AArch64::ZPR4RegClassID).contains(Reg) ||
           MRI.getRegClass(AArch64::PPRRegClassID).contains(Reg))
----------------
Is this necessary? Is it possible for a register to be in `AArch64::ZPR2RegClassID` but not in `AArch64::ZPRRegClassID`? I'm just wondering why we've not hit this before.


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCCodeEmitter.cpp:196-198
+  uint32_t EncodePPR_3b_p8_p15(const MCInst &MI, unsigned OpIdx,
+                               SmallVectorImpl<MCFixup> &Fixups,
+                               const MCSubtargetInfo &STI) const;
----------------
This looks like a bogus rebase issue due to the function being renamed to `EncodePPR_p8to15`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136172