[PATCH] D117900: [AArch64][SVE] Fold gather/scatter with 32bits when possible

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

In D117900#3279405 <https://reviews.llvm.org/D117900#3279405>, @CarolineConcatto wrote:

> - Rebase the patch under D118459 <https://reviews.llvm.org/D118459> and address comments

Can you add D118459 <https://reviews.llvm.org/D118459> as a dependence to this patch (set parent revision)

Thanks for addressing all my comments, the patch looks good to me with final nits addressed.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16178
+
+  // Earlier returns because there is no fold change.
+  if (Stride == 0)
----------------
nit: s/Earlier returns [...] fold change./Return early because no supported pattern is found./


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16198
+
+  // Only use Stride when it knows it does not overflow in MGATHER/MSCATTER
+  EVT NewIndexVT = IndexVT.changeVectorElementType(MVT::i32);
----------------
This comment seems misplaced. I think you can remove it, because the code above where it checks the value of LastElementOffset says as much.


================
Comment at: llvm/test/CodeGen/AArch64/sve-gather-scatter-addr-opts.ll:6
+; Ensure we use a "vscale x 4" wide scatter for the maximum supported offset.
+define void @scatter_i8_index_offset_maximum(i8* %base, i64 %offset, <vscale x 4 x i1> %pg, <vscale x 4 x i8> %data) #0 {
+; CHECK-LABEL: scatter_i8_index_offset_maximum:
----------------
All these tests use scatters, although the name suggests it should work for gathers too. Can you also add at least one positive test that uses a llvm.masked.gather?


================
Comment at: llvm/test/CodeGen/AArch64/sve-gather-scatter-addr-opts.ll:51
+
+; As scatter_f16_index_offset_maximum but with a variable stride that we cannot prove
+; will not wrap when shrunk to be i32 based.
----------------
nit: This test still does not exist in this file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117900