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

[Caroline via Phabricator via llvm-commits]

CarolineConcatto added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1800
+    case 2:
+      if ((getVectorListStart() - AArch64::Z0) < 16)
+        Inst.addOperand(MCOperand::createReg(
----------------
paulwalker-arm wrote:
> 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.
I believe you want to check if the first register is in range, correct? If so, then I believe I have addressed your comment.


================
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;
----------------
paulwalker-arm wrote:
> This looks like a bogus rebase issue due to the function being renamed to `EncodePPR_p8to15`.
I remove the function, but forgot the header.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:3724-3735
+class sme2_ld_vector_vg4_multi_scalar_scalar<bits<2> msz, bit n,
+                                             RegisterOperand multi_vector_ty,
+                                             RegisterOperand gpr_ty,
+                                             string mnemonic>
+  : sme2_ld_vector_vg24_multi_scalar_scalar<msz, n, multi_vector_ty, gpr_ty,
+                                            mnemonic> {
+   bits<3> Zt;
----------------
paulwalker-arm wrote:
> I'm going to surprise/annoy you here but there's more going on here than I typically like.  Do you mind not having the vg24 base classes for the LD1/ST1 instructions in this patch and just have the vg2/vg4 classes fully define `Inst`?
I believe you only want for the scalar-scalar instruction, is that correct?
The scalar+immediate I need the multiclass so I believe it is ok to have some bit defined there. 
I have change to use your {?,?} scheme.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136172