[PATCH] D135599: [AArch64]SME2 Single and multiple vectors SVE Destructive two/four registers[part2]

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

Thanks @CarolineConcatto, this is a much nicer structure and in keeping with how we typically add new instructions.



================
Comment at: llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td:376
+defm SMAX_VG2_2Z2Z : sme2_int_sve_destructive_vector_vg2_multi<"smax",  0b000000>;
+defm SMAX_VG4_4Z2Z : sme2_int_sve_destructive_vector_vg4_multi<"smax",  0b000000>;
+
----------------
Should this be `SMAX_VG4_4Z4Z`? If yes then you'll need to check all new defs in this patch because I've seen this potentially incorrect naming several times.

Whilst mentioning consistency the choice of whether to use `_VG#` seems arbitrary or perhaps I'm misinterpreting it's meaning.  For example `SRSHL_2Z2Z` looks like it should have `_VG2` but doesn't.  That said, given we specify the register types within the instructions (i.e. _2Z2Z) I'm wondering if most all the `_VG#` usages are redundant?

Redundancy is likely a question for later but I do think you should be naming the instructions consistently.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:1439-1441
+  let Inst{10-9}  = op{6-5};
+  let Inst{8}     = op{4}; //f
+  let Inst{7-5}   = op{3-1};
----------------
Is there value in breaking this up? instead of just having `Inst{10-5} = op{6-1}`.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:1475-1477
+  let Inst{10-9}  = op{6-5};
+  let Inst{8}     = op{4}; //f
+  let Inst{7-5}   = op{3-1};
----------------
As above, do these need to be split?


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:1511-1513
+  let Inst{10-9}  = op{5-4};
+  let Inst{8}     = f;
+  let Inst{7-5}   = op{3-1};
----------------
Can you make `f` part of the opcode? much like you did for the _single variants.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:1546-1548
+  let Inst{10-9}  = op{5-4};
+  let Inst{8}     = f;
+  let Inst{7-5}   = op{3-1};
----------------
As above, can these be packed together?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135599