[PATCH] D69128: [AArch64][SVE] Add patterns for some integer vector instructions

[Graham Hunter via Phabricator via llvm-commits]

huntergr added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:187
+
+    for (auto VT : { MVT::nxv16i8, MVT::nxv8i16, MVT::nxv4i32, MVT:: nxv2i64 }) {
+      setOperationAction(ISD::SADDSAT, VT, Legal);
----------------
Nit: shouldn't be a space after `MVT::` for the final valuetype.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:1809
 
-multiclass sve_int_bin_pred_log<bits<3> opc, string asm> {
-  def _B : sve_int_bin_pred_arit_log<0b00, 0b11, opc, asm, ZPR8>;
-  def _H : sve_int_bin_pred_arit_log<0b01, 0b11, opc, asm, ZPR16>;
-  def _S : sve_int_bin_pred_arit_log<0b10, 0b11, opc, asm, ZPR32>;
-  def _D : sve_int_bin_pred_arit_log<0b11, 0b11, opc, asm, ZPR64>;
+multiclass sve_int_bin_pred_log<bits<3> opc, string asm, SDPatternOperator op> {
+  def _B : sve_int_bin_pred_arit_log<0b00, 0b11, opc, asm, nxv16i8, nxv16i1, ZPR8, op>;
----------------
For this encoding set we left the sdag pattern out of the class itself and used the helper `SVE_3_Op_Pat` instead.

e.g.
```
multiclass sve_int_bin_pred_log<bits<3> opc, string asm, SDPatternOperator op> {
  def _B : sve_int_bin_pred_arit_log<0b00, 0b11, opc, asm, ZPR8>;
  def _H : sve_int_bin_pred_arit_log<0b01, 0b11, opc, asm, ZPR16>;
  def _S : sve_int_bin_pred_arit_log<0b10, 0b11, opc, asm, ZPR32>;
  def _D : sve_int_bin_pred_arit_log<0b11, 0b11, opc, asm, ZPR64>;

  def : SVE_3_Op_Pat<nxv16i8, op, nxv16i1, nxv16i8, nxv16i8, !cast<Instruction>(NAME # _B)>;
  def : SVE_3_Op_Pat<nxv8i16, op, nxv8i1, nxv8i16, nxv8i16, !cast<Instruction>(NAME # _H)>;
  def : SVE_3_Op_Pat<nxv4i32, op, nxv4i1, nxv4i32, nxv4i32, !cast<Instruction>(NAME # _S)>;
  def : SVE_3_Op_Pat<nxv2i64, op, nxv2i1, nxv2i64, nxv2i64, !cast<Instruction>(NAME # _D)>;
}
```

In this case there's not much difference between approaches (both fulfil our goal of keeping basic patterns in SVEInstrFormats.td), but sometimes we need to ignore types and just match against register classes, or add in default predicates when matching unpredicated operations. Making more use of the helpers should make the code more maintainable, though our earlier codebase shared on github wasn't using them consistently.

I appreciate we haven't documented our intended use of helper functions or codegen conventions; I'll make a note to encourage more comments on them in future patches.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:3085
+  def NAME : sve_int_bin_cons_log<opc, asm, op>;
+
+  def : SVE_2_Op_Pat<nxv16i8, op, nxv16i8, nxv16i8, !cast<Instruction>(NAME)>;
----------------
I see that you have used the helper here (and thanks for adding the 2-op variant), but you could also use the helper for the nxv2i64 pattern as well instead of adding a set pattern to the encoding class.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:3091
   def : InstAlias<asm # "\t$Zd, $Zn, $Zm",
-                  (!cast<Instruction>(NAME) ZPR8:$Zd,  ZPR8:$Zn,  ZPR8:$Zm),  1>;
+         (!cast<Instruction>(NAME) ZPR8:$Zd,  ZPR8:$Zn,  ZPR8:$Zm),  1>;
   def : InstAlias<asm # "\t$Zd, $Zn, $Zm",
----------------
Not sure why these lines were changed.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69128