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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td:666
+defm ST1D_VG2_M2ZPXI : sme2_st_vector_vg2_multi_scalar_immediate<0b11, 0b0,  ZZ_d_strided,   simm4s2, "st1d">;
+defm ST1D_VG4_M4ZPXI : sme2_st_vector_vg4_multi_scalar_immediate<0b11, 0b0, ZZZZ_d_strided,  simm4s4, "st1d">;
+
----------------
indent me


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1792
+    case 2:
+      if ((getVectorListStart() - AArch64::Z0) < 16) {
+        assert(((getVectorListStart() - AArch64::Z0) < 8) &&
----------------
Does `getVectorListStart() < AArch64::Z16` not work? Seems like it would make this function more readable.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:3764-3767
+  bits<4> Zt;
+  let Inst{4} = Zt{3};
+  let Inst{2-0} = Zt{2-0};
+}
----------------
indent me


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:3769-3770
+
+   def : InstAlias<mnemonic # "\t$Zt, $PNg/z, [$Rn, $imm4, mul vl]",
+                  (!cast<Instruction>(NAME) multi_vector_ty:$Zt, PNRAny_p8to15:$PNg, GPR64sp:$Rn, index_ty:$imm4), 0>;
+
----------------
What does this InstAlias cover? too me it looks identical to the format in `sme2_ld_vector_vg24_multi_scalar_immediate`. The same question likely applies to the other multiclasses.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:3783-3786
+   bits<3> Zt;
+   let Inst{4} = Zt{2};
+   let Inst{1-0} = Zt{1-0};
+}
----------------
indent me


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:3841-3842
+   let Inst{4}     = Zt{2};
+   let Inst{3}     = n;
+   let Inst{1-0}   = Zt{1-0};
+
----------------
This is missing `let Inst{2} = 0b0;`.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:3877-3880
+  bits<4> Zt;
+  let Inst{4}   = Zt{3};
+  let Inst{2-0} = Zt{2-0};
+}
----------------
indent me


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:3895-3898
+   bits<3> Zt;
+   let Inst{4}   = Zt{2};
+   let Inst{1-0} = Zt{1-0};
+}
----------------
indent me


================
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;
----------------
CarolineConcatto wrote:
> 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.
Yes that's fine, thanks for making the change @CarolineConcatto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136172