[PATCH] D136077: [AArch64]SME2 Outer Product and Accumulate instructions

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

I'm only really bothered about the instruction names.  My other suggestions are nice to have but only if you agree they'll make things better.



================
Comment at: llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td:554
 
+defm BMOPA_TPPZZ : sme2_bfp_mopx_tile<"bmopa", 0b0>;
+defm BMOPS_TPPZZ : sme2_bfp_mopx_tile<"bmops", 0b1>;
----------------
Should this be `BMOPA_MPPZZ_S` because it uses ZA not ZT0?


================
Comment at: llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td:557
+
+defm SMOPA_TPPZZ : sme2_int_mopx_tile<"smopa", 0b00>;
+defm SMOPS_TPPZZ : sme2_int_mopx_tile<"smops", 0b01>;
----------------
The equivalent SME1 instruction is `SMOPA_MPPZZ_S` so I'd expect this to be named similar but much like with the DOT instructions you'll need to rename the original to reflect the difference between 4way and 2way.  So the original becomes `SMOPA_MPPZZ_BtoS and SMOPA_MPPZZ_HtoD` and this new one becomes `SMOPA_MPPZZ_HtoS`.  Do you agree?


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:90
 
-class sme_int_outer_product_inst<bit u0, bit u1, bit S, bit sz,
+class sme_int_outer_product_inst<bit u0, bit u1, bit S, bit sz, bit sme2,
                                  MatrixTileOperand za_ty, ZPRRegOp zpr_ty,
----------------
Up to you but given the change perhaps `bit u0, bit u1, bit S` is better expressed as a 3bit opcode?  It seems weird seeing `opc{2}, opc{1}, opc{0}`.  I'm almost tempted to suggest all the operands are just a 5 bit opcode but I'll leave it up to you if you want to make such a change. 


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:148
 
-class sme_outer_product_widening_inst<bit op, bit S, string mnemonic>
+class sme_outer_product_widening_inst<bit op, bit S, bit sme2, ZPRRegOp zpr_ty, string mnemonic>
     : I<(outs TileOp32:$ZAda),
----------------
Again it's up to you but this just looks like a 3bit opcode to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136077