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

[Florian Hahn via Phabricator via llvm-commits]

fhahn 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");
+
----------------
Ayal wrote:
> 
Reordered, thanks!


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


================
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
----------------
Ayal wrote:
> 
Fixed, thanks!


================
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.
----------------
Ayal wrote:
> 
added, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9207
+      if (IsFMulAdd) {
+        assert((MinVF.isScalar() || isa<VPWidenCallRecipe>(CurrentLink)) &&
+               CurrentLink->getOperand(2) == PreviousLinkV &&
----------------
Ayal wrote:
> If VF=1 CurrentLink should be a Replicate instead of a Widen(Call) recipe?
> 
> Better place the above assert(RecurrenceDescriptor::isFMulAddIntrinsic(CurrentLinkI)) here?
Moved assert and refined condition as suggested, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9233
         // If the instruction is a call to the llvm.fmuladd intrinsic then we
         // need to create an fmul recipe to use as the vector operand for the
         // fadd reduction.
----------------
Ayal wrote:
> 
Adjusted, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9244
+      VPRecipeBase *CompareRecipe =
+          CurrentLink->getOperand(0)->getDefiningRecipe();
+
----------------
Ayal wrote:
> 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.
Yep, updated, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155845