[PATCH] D125193: [SVE] Enable use of 32bit gather/scatter indices for fixed length vectors

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4563
+  // Indices cannot be smaller than the main data type.
+  if (IndexVT.getScalarSizeInBits() < DataVT.getScalarSizeInBits())
+    return false;
----------------
david-arm wrote:
> It feels like we may have been missing a SVE test before for the case when IndexVT=<vscale x 4 x i32> and DataVT=<vscale x 4 x i64>. This is a hole that you've fixed, but no tests broke. :)
`vscale x 4 x i64` isn't a legal type so this line doesn't have much of a visible effect for scalable vectors.  It's just going to stop us from removing an extend that we'll later re-add during type legalisation.  After which DataVT will be  `vscale x 2 x i64` and so the original `>=4` clause would have kicked in.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4568
+  // element container type, which would violate the previous clause.
+  return DataVT.isFixedLengthVector() || DataVT.getVectorMinNumElements() > 2;
 }
----------------
david-arm wrote:
> Given we can have types such as <vscale x 3 x > is it worth changing this to
> 
>   DataVT.getVectorMinNumElements() >= 4
> 
> I guess such types will be widened to the next power of 2, but it looks like we might call this before legalisation?
The `<vscale x 3 x ...>` case is why I changed from the original `>=4` to `>2` because as you say, it'll be widened to the next power of 2 (i.e. 4) and thus can benefit from the 32bit addressing modes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125193