[PATCH] D130692: [AArch64][SVE] Expand gather index to 32 bits instead of 64 bits

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17873
+  if (ISD::isVectorShrinkable(Index.getNode(), 32, N->isIndexSigned()) &&
+      !(DataVT.getScalarSizeInBits() == 64 && DataVT.isFixedLengthVector())) {
     EVT NewIndexVT = IndexVT.changeVectorElementType(MVT::i32);
----------------
MattDevereau wrote:
> paulwalker-arm wrote:
> > Please pull this out into a separate check before this block, along with a suitable comment.  The "fix" is not really related to `isVectorShrinkable`, it is just that today that is the only logic applicable for fixed length vectors. However, this might change in the future hence why I prefer an isolated check.
> I'm not sure if I'm missing something obvious here but I don't understand what you're expecting this to look like when this check is seperated. The way this function's logic is structured means it's difficult to bail out cleanly from individual checks here, as we want to fall through this if block instead of returning false. I could use a nested if block, e.g.
> ```
> //comment
> if (!(DataVT.getScalarSizeInBits() == 64 && DataVT.isFixedLengthVector())){
>   if (!ISD::isVectorShrinkable... ){
>     ...
>   }
> }
> ```
> To sort of separate the "temporary" condition, but I believe it still needs to be `and` logic for the correct behaviour here
> Could you please elaborate on what you're expecting?
I was thinking of:
```
// Fixed length vectors are always "packed" so there's no value in the index having a smaller element type than the data.
if (DataVT.isFixedLengthVector() && DataVT.getScalarSizeInBits() == 64)
  return Changed;
```
It is my assertion that we don't want to fall through because all code after this point is trying to rewrite `Index` to be a vector of i32s, which is never going to be good for fixed length vector types because `Index` will just get re-extended during operation legalisation.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130692