[PATCH] D136142: [AArch64]SME2 MOV Instructions
Paul Walker via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 7 07:36:33 PST 2022
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
More information about the llvm-commits
mailing list