[PATCH] D120894: [AArch64][SVE]Make better use of gather/scatter when inside a loop body

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12243
+    return false;
+  for (const Use &U : I->uses()) {
+    auto *UI = cast<Instruction>(U.getUser());
----------------
Because you iterate all uses here, it will look at all uses for the first entry to this function, and later on only check a single use (because of the `if (!UI->hasOneUse()) return false;`). I don't think that's your intention.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12309-12313
+      if ((II && II->getIntrinsicID() == Intrinsic::experimental_stepvector) ||
+          isSplatShuffle(I->getOperand(0)) ||
+          isSplatShuffle(I->getOperand(1))) {
+        for (uint64_t i = 0; i < I->getNumOperands(); i++) {
+          if (isSplatShuffle(I->getOperand(i))) {
----------------
This seems to be doing things the wrong way around. It first checks the operands 0 and 1 individually, and then goes into a loop to go over all the operands to check them again.

I'm thinking more of something like this:

  for (unsigned J = 0; J < I->getNumOperands(); ++J) {
    Use &U = I->getOperandUse(J);
    if (isSplatShuffle(U.get())) {
      Ops.push_back(&cast<Instruction>(U.get())->getOperandUse(0));
      Ops.push_back(&U);
    } else if (auto *II = dyn_cast<IntrinsicInst>(U.get()))
      if (II->getIntrinsicID() == Intrinsic::experimental_stepvector)
        Ops.push_back(&U);
  }


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12320
+        }
+        return true;
+      }
----------------
If it did not find anything to sink based on the above criteria, the code below may still be applicable. So maybe you can query the Ops.size() before/after this loop, and only return `true` if it is different (because you've added new operands).


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12306
+    // and not check stepvector
+    IntrinsicInst *II = dyn_cast<IntrinsicInst>(I->getOperand(1));
+    if (II && II->getIntrinsicID() == Intrinsic::experimental_stepvector) {
----------------
CarolineConcatto wrote:
> sdesmalen wrote:
> > This doesn't need to be limited to the second operand. You can just combine it with the case for splat.
> Step vector will always be the second operand, because the work you did in 
> https://reviews.llvm.org/D118459
In D118459 it canonicalizes `binop(splat(x), step_vector) -> binop(step_vector, splat(x))`, so that we can recognize `binop(step_vector, splat(x))` directly.

At this point you'll want to push down the stepvector intrinsic regardless of which operand it is used in, because if it happens to be used in operand 0, we want to recognize that case as well. If it's in operand 1 and you push it down, SelectionDAG will swap the operands.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120894