[PATCH] D149627: [NFC][SLP] Cleanup: Simplify traversal loop in SLPVectorizerPass::vectorizeHorReduction().

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:14006
+  Value *Op1 = nullptr;
+  matchRdxBop(I, Op0, Op1);
+  assert(Op0 && Op1 && "Expected Binop");
----------------
Better:
```
if (!match(...))
  return nullptr;
```


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:14081-14084
+      Instruction *FutureSeed = Inst;
+      if (TryOperandsAsNewSeeds && Inst == Root) {
+        FutureSeed = getNonPhiOperand(Root, P);
+        if (!FutureSeed)
----------------
This logic also better to represent as lambda


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:14008-14010
+  Instruction *Op = dyn_cast<Instruction>(Op0);
+  if (Op == Phi)
+    Op = dyn_cast<Instruction>(Op1);
----------------
vporpo wrote:
> ABataev wrote:
> > ABataev wrote:
> > > `auto *Op`
> > `return dyn_cast<Instruction>(Op0 == Phi ? Op1 : Op0);`
> Phi and Op are already Instruction * so I will use this instead: `return Op == Phi ? dyn_cast<Instruction>(Val: Op1) : Op; `
You just don't need Op at all, just 
`return dyn_cast<Instruction>(Op0 == Phi ? Op1 : Op0);`
after assert is enough.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:14083
+        if (!FutureSeed)
+          break;
       }
----------------
vporpo wrote:
> ABataev wrote:
> > Why break here?
> This can only happen if Inst is the root node and at this point `Stack` is empty so it effectively breaks out of the loop, so it is better to be explicit about it.
Add an `assert(Stack.empty())` here and check with at least Spec that it does not crash


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149627