[PATCH] D120912: [AArch64][SVE] Convert gather/scatter with a stride of 2 to contiguous loads/stores

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Hi @kmclaughlin, this looks like a nice improvement! I've not reviewed all of it so far, but I'll leave a few comments in the bits I have reviewed.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16626
+  }
+
+  if (Index->getOpcode() == ISD::STEP_VECTOR &&
----------------
Perhaps worth adding a comment here explaining that the following code will attempt to transform stride=2 gathers/scatters into contiguous loads/stores?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16627
+
+  if (Index->getOpcode() == ISD::STEP_VECTOR &&
+      Mask->getOpcode() != ISD::EXTRACT_SUBVECTOR &&
----------------
It might look nicer if you can avoid unnecessary indentation here by rewriting this to return early, i.e.

  if (Index->getOpcode() != ISD::STEP_VECTOR ||
      Mask->getOpcode() == ISD::EXTRACT_SUBVECTOR ||
      IndexType != ISD::SIGNED_SCALED)
    return SDValue();

  ... rest of code with less indentation ...



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16632
+
+    if (!Step || Step != 2)
+      return SDValue();
----------------
I think you can just write `if (Step != 2)` here.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16636
+    unsigned ShiftAmt;
+    switch (MemVT.getSimpleVT().SimpleTy) {
+    case MVT::nxv2i64:
----------------
Are you assuming this is a legal/simple type here? For example, the type could be <vscale x 12 x i32>, which will be an extended type and `getSimpleVT` will assert. Could you add a test case for this? I think you can avoid this either by only doing this after legalisation or by rewriting as a sequence of if-else statements, i.e.

  if (MemVT == MVT::nxv2i64 || MemVT == MVT::nxv2f64
  ...



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120912