[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>;
----------------
CarolineConcatto wrote:
> paulwalker-arm wrote:
> > Just related to layout but it's more typical to group instructions based on the multiclass rather instruction name.
> Do you want me to group them in this patch by group of instructions?
> Sorry, but I don't understand what you would like me to fix.
> I can group them here, and do another NFC for the other instructions, otherwise things will get a bit moew random.
If you look at AArch64SVEInstrInfo.td you'll see that typically we group things based on class names, so in this case I'd expect
```
defm SMAX_VG2_2ZZ  : sme2_int_max_min_vector_vg2_single<"smax", 0b00>;
defm UMAX_VG2_2ZZ  : sme2_int_max_min_vector_vg2_single<"umax", 0b01>;
defm SMIN_VG2_2ZZ  : sme2_int_max_min_vector_vg2_single<"smin", 0b10>;
defm UMIN_VG2_2ZZ  : sme2_int_max_min_vector_vg2_single<"umin", 0b11>;

defm SMAX_VG4_4ZZ  : sme2_int_max_min_vector_vg4_single<"smax", 0b00>;
defm UMAX_VG4_4ZZ  : sme2_int_max_min_vector_vg4_single<"umax", 0b01>;
defm SMIN_VG4_4ZZ  : sme2_int_max_min_vector_vg4_single<"smin", 0b10>;
defm UMIN_VG4_4ZZ  : sme2_int_max_min_vector_vg4_single<"umin", 0b11>;
```
However this is purely stylistic and I can see AArch64SMEInstrInfo.td uses a mixture of styles so this can be cleaned up in a following patch or ignored given I suppose there's no rule to say both should follow the same format.


================
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>
----------------
CarolineConcatto wrote:
> paulwalker-arm wrote:
> > 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.
> I created a class for each group now.
>  I hope that is better now to review.
Thanks Carol, this helps a lot.  The similarities are clearer which prompted me to look at the encoding groups and I think you've perhaps gone one level too far down?  I can see the encoding group "SME2 Multi-vector - Multiple and Single SVE Destructive (Two registers)" which these instructions below, amongst others which all use the same format and so perhaps you can have one instruction class which all the similar instructions (min, max, fmin, fmax, shift, add...) can use.  Sure the opcode space is sparse but that's fine. It'll just mean it's easier to add a new instruction within this group later on.

The same goes for the "Four registers" variant where I see "SME2 Multi-vector - Multiple and Single SVE Destructive (Four registers)", so again one instruction class can likely be used for all the instructions it contains.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135599