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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for splitting this patch up, that makes it a bit easier to review!



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16116
+      Index.getOperand(1).getOpcode() == ISD::STEP_VECTOR) {
+    auto *C = dyn_cast<ConstantSDNode>(Index.getOperand(1).getOperand(0));
+    if (!C)
----------------
nit: can you clarify this by adding a variable `SDValue StepVector = Index.getOperand(1)`, and then querying:
  auto *C = cast<ConstantSDNode>(StepVector.getOperand(0))


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16116-16118
+    auto *C = dyn_cast<ConstantSDNode>(Index.getOperand(1).getOperand(0));
+    if (!C)
+      return false;
----------------
sdesmalen wrote:
> nit: can you clarify this by adding a variable `SDValue StepVector = Index.getOperand(1)`, and then querying:
>   auto *C = cast<ConstantSDNode>(StepVector.getOperand(0))
the operand to a stepvector is always a constant, so it will never return false here.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16119
+      return false;
+    Stride = C->getSExtValue();
+    if (auto Offset = DAG.getSplatValue(Index.getOperand(0))) {
----------------
`Stride` should //only// be set if `Offset` is a splat value, otherwise the return value is incorrect.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16119
+      return false;
+    Stride = C->getSExtValue();
+    if (auto Offset = DAG.getSplatValue(Index.getOperand(0))) {
----------------
sdesmalen wrote:
> `Stride` should //only// be set if `Offset` is a splat value, otherwise the return value is incorrect.
Stride should only be set in the block below (under `if (auto Offset = DAG.getSplatValue(Index.getOperand(0)))` because otherwise the function returns `true` (Stride != 0), but `BasePtr` will be undefined.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16128
+  if (Stride < std::numeric_limits<int32_t>::min() ||
+      Stride > std::numeric_limits<int32_t>::max())
+    return false;
----------------
This should also bail out early if `Stride == 0`


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16104
+                                     SelectionDAG &DAG) {
+  bool ScaledOffset =
+      (IndexType == ISD::SIGNED_SCALED) || (IndexType == ISD::UNSIGNED_SCALED);
----------------
sdesmalen wrote:
> nit: The word 'Offset' is used quite a few times in this function, which confused me a bit. I think what this is saying is that the eventual instruction will scale the offset (i.e. the offset is not an offset, but an index). So maybe change this to `OffsetIsScaledByInstruction`?
This comment related to the variable `ScaledOffset` you had in the previous revision of this patch, but then you found out that variable could be removed, so this comment then became redundant.

I think the function should be named `findMoreOptimalIndexType`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117900