Page MenuHomePhabricator

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

Authored by fhahn on Apr 8 2021, 8:03 AM.

Details

Reviewers
Ayal
gilr
rengolin
Summary

All information to fix-up the reduction phi nodes in the vectorized loop
is available in VPlan now. This patch moves the code to do so, to make
this clearer. Fixing up the loop exit value still relies on other
information and remains outside of VPlan for now.

Diff Detail

Event Timeline

fhahn created this revision.Apr 8 2021, 8:03 AM
fhahn requested review of this revision.Apr 8 2021, 8:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2021, 8:03 AM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal added inline comments.Mon, May 3, 12:47 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9418–9419

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.

llvm/lib/Transforms/Vectorize/VPlan.cpp
63

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.

820

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.

fhahn updated this revision to Diff 343863.Sat, May 8, 1:54 PM

Rebased and updated the code to check whether to use ordered reductions during codegen to get the VPReductionRecipe and check isOrdered on the reduction descriptr. This builds on the code to determine whether a VPWidenPHI is part of an in-loop reduction.

fhahn added inline comments.Sat, May 8, 1:58 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9418–9419

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.

llvm/lib/Transforms/Vectorize/VPlan.cpp
63

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.

820

I rebased the code, and it now uses getVPSingleValue/getBackedgeValue. I can add the additional comments separately or in this change.