[PATCH] D135599: [AArch64]SME2 Single and multiple vectors Int/FP min/max two/four registers

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td:374-384
+defm SMAX_VG2_2ZZ  : sme2_int_max_min_vector_vg2_single<"smax", 0b00>;
+defm SMAX_VG4_4ZZ  : sme2_int_max_min_vector_vg4_single<"smax", 0b00>;
+defm SMAX_VG2_2Z2Z : sme2_int_max_min_vector_vg2_multi<"smax",  0b00>;
+defm SMAX_VG4_4Z2Z : sme2_int_max_min_vector_vg4_multi<"smax",  0b00>;
+
+defm UMAX_VG2_2ZZ  : sme2_int_max_min_vector_vg2_single<"umax", 0b01>;
+defm UMAX_VG4_4ZZ  : sme2_int_max_min_vector_vg4_single<"umax", 0b01>;
----------------
Just related to layout but it's more typical to group instructions based on the multiclass rather instruction name.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:1831-1832
+
+class sme2_max_min_vector_vg24<bits<2> sz, bit f, bits<2> op,
+                               RegisterOperand vector_ty,
+                               RegisterOperand zpr_ty, string mnemonic>
----------------
There doesn't look to be much commonality between the derived classes and the splitting of the `Inst` fields makes it hard to spot bugs.  I guess I'm asking why the base class is advantageous over implementing `sme2_max_min_vector_vg2_single`, `sme2_max_min_vector_vg4_single`,... as isolated instruction classes.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:1853
+    : sme2_max_min_vector_vg24<sz, f, op, vector_ty,zpr_ty,
+                               mnemonic> {
+  bits<4> Zm;
----------------
Is the line wrapping here necessary? Typically we only follow the 80 character limit for these td files when it comes to comments and class declarations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135599