[PATCH] D69956: [AArch64][SVE] Integer reduction instructions pattern/intrinsics.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.
This revision is now accepted and ready to land.

Thanks for making these changes to your patch @dancgr.
Please address the two nits before you commit, but otherwise LGTM.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10521
+
+  if (DataVT.getVectorElementType().isScalarInteger() && (VT == MVT::i8 || VT == MVT::i16 || VT == MVT::i32 || VT == MVT::i64)) {
+    auto bitSize = VT.getSizeInBits();
----------------
nit: >80chars. Please use clang-format before you commit.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10522
+  if (DataVT.getVectorElementType().isScalarInteger() && (VT == MVT::i8 || VT == MVT::i16 || VT == MVT::i32 || VT == MVT::i64)) {
+    auto bitSize = VT.getSizeInBits();
+
----------------
nit: `BitSize` is not very descriptive. Perhaps because it is used only once, just propagate `VT.getSizeInBits()` into the expression calculating `OutputVT`?
nit: The first character of variables in this file are capitalized, so this should have been `BitSize`.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:67
+  defm SADDV_VPZ : sve_int_reduce_0_saddv<0b000, "saddv", int_aarch64_sve_saddv>;
+  defm UADDV_VPZ : sve_int_reduce_0_uaddv<0b001, "uaddv", int_aarch64_sve_uaddv, int_aarch64_sve_saddv>;
+  defm SMAXV_VPZ : sve_int_reduce_1<0b000, "smaxv", AArch64smaxv_pred>;
----------------
This reads a bit confusing, but I guess it works. You could have done the same thing for all intrinsics. By calling `LowerSVEIntReduction` and using the same pattern for all reductions, rather than one using the `SVE_2_Op_Pat` for `uaddv/saddv`, and using `SVE_2_Op_Pat_Reduce_To_Neon` for the others. The only thing different is the result type (i64 vs i8/i16/i32/i64), but the same lowering function works for both.


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

https://reviews.llvm.org/D69956