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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm accepted this revision.
paulwalker-arm added a comment.
This revision is now accepted and ready to land.

I still want `BMOPA_MPPZZ/BMOPS_MPPZZ` to include an element suffix but otherwise looks good.



================
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>;
----------------
CarolineConcatto wrote:
> paulwalker-arm wrote:
> > Should this be `BMOPA_MPPZZ_S` because it uses ZA not ZT0?
> Now, I am not sure if we should do the same for BMOPA.
> In sme1 BFMOPA do not have a size, but FMOPA_MPPZZ has(but because it has a D and S size)
> 
Most all instructions have a size.  It is just that some only support a single element type. In such cases we should still add the size suffix so `_S` in this case because otherwise if a new variant of  bmopa is added later we'll be forced to have to rename stuff, much like with the smopa discussion.


================
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>;
----------------
CarolineConcatto wrote:
> paulwalker-arm wrote:
> > 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?
> I had changed for SME1, but these instructions name are in use in AArch64ISelLowering.
> So I did not changed for sme1, but did for sme2.
> TBH I don't know what is more consistent, changing or not changing the names for sme2.
We should change the old names to be consistent.  It doesn't matter that they are used by AArch64ISelLowering because it's just a textual change. It's not that the old names were wrong when originally added, but based on these new SME2 instructions the old names will lead to confusion, hence the new `_HtoS` style for such things.

With all that said, this assembler work is more important so feel free to defer that refactoring until the assembler support is complete.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136077