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

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau 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);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130692