[PATCH] D100113: [LV] Move reduction PHI node fixup to VPlan::execute (NFC).

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9242
     RecurKind Kind = RdxDesc->getRecurrenceKind();
-    bool IsOrdered = useOrderedReductions(*RdxDesc);
+    bool IsOrdered = LoopVectorizationPlanner::useOrderedReductions(*RdxDesc);
     Value *NewVecOp = State.get(getVecOp(), Part);
----------------
Ayal wrote:
> better turn on an IsOrdered bit inside VPReductionRecipe during its creation or planning, than during its execute? Admittedly goes back to D98435.
> 
> There are 3 types of reductions: (1) parallel lanes: each lane computes an independent partial-sum; (2) parallel parts: each vector computes an independent partial sum, in-loop within vectors but not across parts, and (3) serial: in-loop across vectors and parts, the header phi operates on a single scalar. Would be good to explicitly indicate which type of reduction is chosen, in both ReductionRecipe(s) and the header phi; it effects their code-gen.
> better turn on an IsOrdered bit inside VPReductionRecipe during its creation or planning, than during its execute? Admittedly goes back to D98435.

I think we already have all the required information available with the existing pieces. I updated the patch to check `isOrdered` on the recurrence descriptor of the VPReductionRecipe and a helper to get the VPReductionRecipe for a VPWidenPHIRecipe, if there's any.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:69
 
+bool LoopVectorizationPlanner::useOrderedReductions(
+    RecurrenceDescriptor &RdxDesc) {
----------------
Ayal wrote:
> Defining a method of LVP inside VPlan.cpp seems odd, at best.
> 
> The use of a gating EnableStrictReductions knob (in contrast to a CM cost-based decision), belongs to Legal rather than CM or Planner? I.e., Legal should bail out from vectorizing a loop having an ordered reduction, when !EnableStrictReductions? Admittedly goes back to D98435.
Agreed! I updated the code to check `isOrdered` on the reduction recurrence descriptor directly during code generation. To do so, I evolved the `isInLoopReduction` helper to an `getInLoopReduction` member that returns the `VPReductionRecipe`, if there's one.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:765
+    for (unsigned Part = 0; Part < State->UF; ++Part) {
+      auto *VecRdxPhi = cast<PHINode>(State->get(PhiR->getVPValue(0), Part));
+      Value *Val = State->get(PhiR->getOperand(1), Part);
----------------
Ayal wrote:
> Moving this to VPlan::execute is great!
> Can use getSingleVPValue(), getBackedgeValue().
> May be worth explaining that Val is first set per-part supporting parallel partial sums, either vector/per-lane (1) or scalar/per-part (2), and then set to last-part supporting serial reduction (3).
> Admittedly goes back to D98435.
I rebased the code, and it now uses getVPSingleValue/getBackedgeValue. I can add the additional comments separately or in this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100113