[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:9161
+    if (MinVF.isScalar() && !PhiR->isOrdered())
       continue;
 
----------------
Ayal wrote:
> nit: Should this filtering be applied above when inserting into InLoopReductionPhis?
> Should unordered reductions be considered non-isInLoop in VPlans of VF=1?
Moved the filtering. Left scalar handling as is for now to keep with original code.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9172
+        if (!UserRecipe)
+          continue;
+        assert(UserRecipe->getNumDefinedValues() == 1 &&
----------------
Ayal wrote:
> note: may assert that UserRecipe resides inside the vector loop, as the outside user is a LiveOut, at-least for now.
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9181
+    assert(!RecurrenceDescriptor::isSelectCmpRecurrenceKind(Kind) &&
+           "select/cmp reductions are not allowed for in-loop reductions");
+
----------------
Ayal wrote:
> nit: assert better placed above, asap, independent of filling Worklist.
Moved up, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9194
+
+      // Index of the first operand which holds a non-mask vector operand.
+      unsigned IndexOfFirstOperand;
----------------
Ayal wrote:
> nit: would always considering the last two indices be (correct and) easier than considering either first two indices or indices 1 and 2? Excluding FMulAdd which deserves a separate treatment.
For the initial version, I think it would be good retain the original logic as much as possible. This also makes it easier to retain the asserts.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9203
+        if (auto *Cmp = dyn_cast<VPWidenRecipe>(CurrentLink)) {
+          assert(isa<CmpInst>(Cmp->getUnderlyingInstr()) &&
+                 "need to have the compare of the select");
----------------
Ayal wrote:
> 
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9206
+          continue;
+        }
+        IndexOfFirstOperand = 1;
----------------
Ayal wrote:
> assert CurrentLink is a select recipe (whose first operand index is 1)?
added assert, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9210
+        assert((MinVF.isScalar() || isa<VPWidenRecipe>(CurrentLink) ||
+                (IsFMulAdd && isa<VPWidenCallRecipe>(CurrentLink))) &&
+               "Expected to replace a VPWidenSC or a FMulAdd intrinsic call");
----------------
Ayal wrote:
> Treat FMulAdd separately from the binary operators - the operand along the chain is in index 2, go ahead and generate the FMul of indices 0 and 1 to produce VecOp? (Keeping the assert message intact ;-)
Moved, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9220
+              : IndexOfFirstOperand;
+      VPValue *VecOp = CurrentLink->getOperand(VecOpId);
 
----------------
Ayal wrote:
> assert that VecOp != PreviousLinkV and that the other candidate does equal PreviousLinkV?
> (`CurrentLink->getOperand(IndexOfFirstOperand+IndexOfFirstOperand+1-VecOpId)`)
Added assert, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9225
       VPValue *CondOp = nullptr;
-      if (CM.blockNeedsPredicationForAnyReason(R->getParent())) {
+      if (CM.blockNeedsPredicationForAnyReason(BB)) {
         VPBuilder::InsertPointGuard Guard(Builder);
----------------
Ayal wrote:
> Wonder if this information may already be recorded in VPlan - perhaps BlockInMask already exists?
Not easily yet, but I think as follow-up this could be handled by checking the predecessors of the block to find the predecessor with branch-on-mask.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9235
         // fadd reduction.
         VPInstruction *FMulRecipe = new VPInstruction(
+            Instruction::FMul, {VecOp, CurrentLink->getOperand(1)});
----------------
Ayal wrote:
> assert VecOpId == 0?
Not needed after code movement.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155845