[PATCH] D136142: [AArch64]SME2 MOV 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.

A couple of suggestions but otherwise looks good.  I think my `op3:op4:op5` comment will help with your follow-on work but I'm also happy if you want to delay that until you need the support (i.e. when you add MOVAZ).



================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:1431
 
+// Implicit immediates ranges 0:1 and 0:3, scale has no meaning
+// since the immediate is zero
----------------
`immediate`


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:3306
+  let Inst{12-8} = 0b00100;
+  let Inst{7}    = op;
+  let Inst{4-2}  = Zd;
----------------
Please can this be `Inst{7-5} = op;`? then pass in `{0, ?, ?}` and `{?, ?, ?}` accordingly.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:3366
+                                                !if(v, TileVectorOpV32,
+                                                      TileVectorOpH32),
+                                                MatrixIndexGPR32Op12_15,
----------------
indent me


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:3391
+                                                !if(v, TileVectorOpV32,
+                                                      TileVectorOpH32),
+                                                MatrixIndexGPR32Op12_15,
----------------
indent me


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:3418
+  bits<3> imm;
+  let Inst{31-15} = 0b11000000000001100;
+  let Inst{14-13} = Rs;
----------------
I'm being picky but I think the following better matches the documentation. However, I understand this might be excessively verbose so it's up to you which you prefer.
```
let Inst{31-15} = 0b11000000;
let Inst{23-22} = 0b00; //op0
let Inst{21-19} = 0b000;
let Inst{18} = 0b1; // op1
let Inst{17} = 0b1;
let Inst{16-15} = 0b00; // op2
```

Same goes for `sme2_mova_vec_to_array_vg24_multi`.



================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:3422
+  let Inst{10}    = op{1};
+  let Inst{9-8}   = 0b00;
+  let Inst{7-5}   = imm;
----------------
It'll better reflect the encoding group if you make `op3:op4:op5` be the passed in opcode. I know there's commonality across some of the bits but it makes it easier to match the documentation and it'll just work when you come to add `MOVAZ`.


================
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,
----------------
CarolineConcatto wrote:
> paulwalker-arm wrote:
> > 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.
> We created precendent for that when we created for sme1
> multiclass sme_vector_to_tile_aliases<
> 
> I try to remove the alias class, but one class like this:
> 
> ```
>   defm : sme2_mova_vec_to_tile_or_array_aliases<1, !cast<Instruction>(NAME # _B),
>                                                 !if(v, TileVectorOpV8,
>                                                        TileVectorOpH8),
>                                                 MatrixIndexGPR32Op12_15,
>                                                 uimm3s2range,  ZZ_b_mul_r,
>                                                 "mov">;
> ```
> Would become 2, one for Horizontal and another for Vertical:
> 
> ```
>   def : InstAlias<mnemonic # "\t$ZAd[$Rs, $imm], $Zn",
>                   (!cast<Instruction>(NAME # _B) TileVector8H:$ZAd, rv_ty:$Rs, index_ty:$imm, ZZ_b_mul_r:$Zn), 1>;
>   def : InstAlias<mnemonic # "\t$ZAd[$Rs, $imm], $Zn",
>                   (!cast<Instruction>(NAME # _B) TileVector8V:$ZAd, rv_ty:$Rs, index_ty:$imm, ZZ_b_mul_r:$Zn), 1>;
> ```
> 
> We will have to duplicate the number of alias. 
> And I cannot pass the types (TileVector8H/TileVector8V, TileVector16H/TileVector16V/...)in the previous class :
> 
> ```
> multiclass sme2_mova_vec_to_tile_vg2_multi<string mnemonic>{
>  defm _H : sme2_mova_vec_to_tile_vg2_multi_base<0b0, mnemonic>;
>  defm _V : sme2_mova_vec_to_tile_vg2_multi_base<0b1, mnemonic>;
> }
> ```
> Because it depends on the size.
> 
> Unless you know some tricks in tablegen to solve that problem.
I see, thanks for explaining.  Perhaps the H/V hierarchy is upside down but given this matches the precedent let's stick with what you've got.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136142