[PATCH] D137571: [AArch64] Add all SME2.1 instructions Assembly/Disassembly

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/include/llvm/Support/AArch64TargetParser.h:79
+  AEK_B16B16 =      1ULL << 46, // FEAT_B16B16
+  AEK_SMEF16F16 =   1ULL << 47, // FEAT_SMEF16F16
 };
----------------
Rouge comma at end of list.


================
Comment at: llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td:793
+defm LUTI4_S_2ZTZI : sme2p1_luti4_vector_vg2_index<"luti4">;
+def LUTI4_S_4ZTZI : sme2p1_luti4_vector_vg4_index<"luti4">;
+}
----------------
This is missing a `_H` suffix, although to be honest you'll need a multiclass for code generation support anyway so you may as well follow the usual idiom and make `sme2p1_luti4_vector_vg4_index` a multiclass.


================
Comment at: llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td:796
+
+let Predicates = [HasSME2p1, HasSMEF16F16] in {
+defm FADD_VG2_M2Z_H : sme2_multivec_accum_add_sub_vg2<"fadd", 0b0100, MatrixOp16, ZZ_h_mul_r>;
----------------
Is it correct to require `SME2p1` in order to allow `SMEF16F16`?  The documentation suggests `FEAT_SME_F16F16` is read independently of other feature flags.  Perhaps requiring `SME2` is a better base requirement as this is the feature that added the core support for these instructions.


================
Comment at: llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td:823
+
+let Predicates = [HasSME2p1, HasB16B16] in {
+defm BFADD_VG2_M2Z_H : sme2_multivec_accum_add_sub_vg2<"bfadd", 0b1100, MatrixOp16, ZZ_h_mul_r>;
----------------
Same question as above regarding whether requiring `SME2p1` is too stringent.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:1341-1342
+  let Inst{18}    = op{2};
+  let Inst{17}    = 0b0;
   let Inst{15}    = 0b0;
   let Inst{14-13} = Rv;
----------------
Please can you pass in `vg` so we can have `let Inst{16}    = vg;`


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:2048
 
 // SME2 multi-vec ternary indexed two registers 32-bit
+class sme2_multi_vec_array_vg2_index_32b<bits<5> op, MatrixOperand matrix_ty,
----------------
16/32-bit? Or perhaps move the comment to the 32-bit specific multiclass.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:2049
 // SME2 multi-vec ternary indexed two registers 32-bit
-class sme2_multi_vec_array_vg2_index_32b<bits<4> op,
+class sme2_multi_vec_array_vg2_index_32b<bits<5> op, MatrixOperand matrix_ty,
                                          RegisterOperand multi_vector_ty,
----------------
I think it'll be clearer if you pass in `bit sz` for bit 22 and have a 6-bit opc that represents `{12-10:5-3}`, that way the base instruction class has no holes.

Perhaps drop `_32b`?


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:2146
 
-class sme2_multi_vec_array_vg4_index_32b<bits<4> op,
+class sme2_multi_vec_array_vg4_index_32b<bits<5> op, MatrixOperand matrix_ty,
                                          RegisterOperand multi_vector_ty,
----------------
The same comments I had for `sme2_multi_vec_array_vg2_index_32b` apply here. 


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


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:4259
+
+class sme2p1_zero_matrix<bits<5> opc, Operand index_ty, string mnemonic,
+                         string vg_acronym="">
----------------
I think this class requires `let Constraints = "$ZAd = $_ZAd";`?


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:4270
+  let Inst{12-3} = 0b0000000000;
+  let Inst{2-1}  = opc{1-0};
+}
----------------
Please can you make opc a 6-bit opcode so it matches the encoding group?


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:4320
+  let Inst{31-19} = 0b1100000010011;
+  let Inst{18-17} = op;
+  let Inst{14-13} = 0b10;
----------------
Please can `opc` cover `{18-15}` to better reflect the encoding group?


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:4322
+  let Inst{14-13} = 0b10;
+  let Inst{12}    = sz;
+  let Inst{11-10} = 0b00;
----------------
Please can `sz` remain as 2-bit to match the encoding group?


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:4369
+  let Inst{31-19} = 0b1100000010011;
+  let Inst{18-17} = op;
+  let Inst{15-13} = 0b100;
----------------
Please can `opc` cover `{18-16}` to better reflect the encoding group?


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:4371
+  let Inst{15-13} = 0b100;
+  let Inst{12}    = sz;
+  let Inst{11-10} = 0b00;
----------------
Please can `sz` remain as 2-bit to match the encoding group?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137571