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

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:13985
 /// \Returns the new root if found, which may be nullptr if not an instruction.
-static Instruction *tryGetScondaryReductionRoot(PHINode *Phi,
+static Instruction *tryGetSecondaryReductionRoot(PHINode *Phi,
                                                 Instruction *Root) {
----------------
ABataev wrote:
> 1. Make a separate NFC patch.
> 2. Format, the second parameter is not aligned
OK I will do this in a separate patch.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:14035-14042
+  Instruction *NewRoot = nullptr;
+  if (isReductionCandidate(Root)) {
+    assert((!TryOperandsAsNewSeeds || is_contained(P->operands(), Root)) &&
+           "Phi needs to use the binary operator");
+    if (TryOperandsAsNewSeeds &&
+        HorizontalReduction::getRdxKind(Root) != RecurKind::None)
+      NewRoot = tryGetSecondaryReductionRoot(P, Root);
----------------
ABataev wrote:
> ```
> if (P && isa<BinaryOperator>(Root) && HorizontalReduction::getRdxKind(Root) != RecurKind::None)
>   Root = tryGetSecondaryReductionRoot(P, Root);
> ```
I kept `NewRoot` because we need the original Root for `Inst == Root` later, otherwise the logic changes a bit. I also prefer to use `TryOperandsAsNewSeeds` instead of `P` as it shows intent. Same for `isReductionCandidate()` instead of `isa<BinaryOperator>()`.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:14071
+    Value *VectorizedV = nullptr;
+    if (!R.isAnalyzedReductionRoot(Inst) && isReductionCandidate(Inst)) {
+      HorizontalReduction HorRdx;
----------------
ABataev wrote:
> What if Inst is one of the P operands here, but not Root?
This should work as the original code. This condition is the same as line 14028 and 14032.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:14086
+      Instruction *FutureSeed = Inst;
+      if (TryOperandsAsNewSeeds && Inst == Root) {
+        FutureSeed = getNonPhiOperand(Root, P);
----------------
ABataev wrote:
> I think this is not quite same as before, it should not be a Root, it may be another P operand.
In the original code `P` was set to nullptr after visiting the root node, so this would only enter if `Inst == Root`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149627