[PATCH] D136142: [AArch64]SME2 MOV Instructions

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td:589-592
+defm INSERT_MXI2Z   : sme2_mova_vec_to_tile_vg2_multi<"mova">;
+defm INSERT_MXI4Z   : sme2_mova_vec_to_tile_vg4_multi<"mova">;
+defm EXTRACT_2ZMXI  : sme2_mova_tile_to_vec_vg2_multi<"mov">;
+defm EXTRACT_4ZMXI : sme2_mova_tile_to_vec_vg4_multi<"mov">;
----------------
Should these not be `MOVA/MOV`? the register suffix points to the direction of travel.


================
Comment at: llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td:591
+defm INSERT_MXI4Z   : sme2_mova_vec_to_tile_vg4_multi<"mova">;
+defm EXTRACT_2ZMXI  : sme2_mova_tile_to_vec_vg2_multi<"mov">;
+defm EXTRACT_4ZMXI : sme2_mova_tile_to_vec_vg4_multi<"mov">;
----------------
`mova`?


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:2648
+
+class sme2_mova_vec_to_tile_vg2_multi_base<bits<2> sz, bit v,
+                                           RegisterOperand tile_ty,
----------------
Please add comments to all the base instruction classes that reference their encoding groups.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:2671
+
+multiclass sme2_mova_vec_to_tile_or_array_aliases<int prefer, Instruction inst,
+                                                  RegisterOperand tile_or_array_ty,
----------------
To be honest I don't see any value to this class, especially as it only contains a single `InstAlias`. I think the aliases will be more readable if just emitted directly.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:2839
+    bits<2> imm;
+    let Inst{2}   = 0b0;
+    let Inst{1-0} = imm;
----------------
The vg2 case didn't do this so it's fine but for `sme2_mova_vec_to_tile_vg4_multi_base` please can you pass in the 3-bit opc and then use the `{0, ?, ?}` trick.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:2955
+   bits<4> Zn;
+   let Inst{10} = 0b0;
+   let Inst{9-6} = Zn;
----------------
Can `sme2_mova_vec_to_array_vg24_multi` take a 5-bit opcode and use `{ 0, ?,.. ` and then the vg4 instance will be `{1, ?, ?, ?, 0 }`


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:3289
+    bits<2> imm;
+    let Inst{7}   = 0b0;
+    let Inst{6-5} = imm;
----------------
Same comment as with `sme2_mova_vec_to_tile_vg4_multi_base`.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:3402
+    bits<4> Zd;
+    let Inst{10}  = 0b0;
+    let Inst{4-1} = Zd;
----------------
Same comment as with `sme2_mova_vec_to_array_vg2_multi`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136142