[PATCH] D155845: [VPlan] Fix in-loop reduction chains using VPlan def-use chains (NFCI)

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9165-9172
+    SetVector<VPRecipeBase *> Worklist;
+    Worklist.insert(PhiR);
+
+    RecurKind Kind = RdxDesc.getRecurrenceKind();
+    assert(!RecurrenceDescriptor::isSelectCmpRecurrenceKind(Kind) &&
+           "select/cmp reductions are not allowed for in-loop reductions");
+
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9181
+               "recipes must define exactly one result value");
+        ;
+        Worklist.insert(UserRecipe);
----------------
?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9187
+    // Visit operation "Links" along the reduction chain top-down starting from
+    // the phi until LoopExitValue. We keep a track of the previous item
+    // (PreviousLink) to tell which of the two operands of a Link will remain
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9189
+    // (PreviousLink) to tell which of the two operands of a Link will remain
+    // scalar and which will be reduced. For minmax, Link will be the select
+    // instructions.
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9207
+      if (IsFMulAdd) {
+        assert((MinVF.isScalar() || isa<VPWidenCallRecipe>(CurrentLink)) &&
+               CurrentLink->getOperand(2) == PreviousLinkV &&
----------------
If VF=1 CurrentLink should be a Replicate instead of a Widen(Call) recipe?

Better place the above assert(RecurrenceDescriptor::isFMulAddIntrinsic(CurrentLinkI)) here?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9244
+      VPRecipeBase *CompareRecipe =
+          CurrentLink->getOperand(0)->getDefiningRecipe();
+
----------------
fhahn wrote:
> Ayal wrote:
> > Could we use PreviousLink serve as/instead of CompareRecipe?
> Unfortunately I don't think so, but we can let dead-recipe removal take care of this removal. I removed the code.
If PreviousLink is set to Cmp before early-continuing above?

OTOH, dead-recipe removal could also reclaim CurrentLink.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155845