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

Sat Oct 22 05:03:27 PDT 2022

paulwalker-arm added inline comments.

================
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>
----------------
paulwalker-arm wrote:
> 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.
Just to avoid confusion here.  I'm not saying we don't need the multiclasses like sme2_fp_max_min_vector_vg2_single and sme2_int_max_min_vector_vg2_single.  We need those to account for the differences in supported element sizes. However, I think they can inherit from the same "SME2 Multi-vector - Multiple and Single SVE Destructive (Two registers)"  instruction class.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135599