This patch updates VPWidenPHI recipes for first-order recurrences to
also track the incoming value from the back-edge. Similar to D99294,
which did the same for reductions.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
4751–4752 | This does raise an eyebrow... rename StartVPV and StartV to something like ReductionStartVPV and ReductionStartV? Only start values of reductions are handled here, those of firstOrderRecurrences are handled in fixFirstOrderRecurrence(). Better yet to first handle firstOrderRecurrences, separately? It's just creating the phi (which will eventually be replaced by the one fixFirstOrderRecurrence() creates) and setting the state, per part. | |
8978 | This StartV for non-reduction phi's is currently not being used, except perhaps as a placeholder to ensure backedge takes 2nd place, right? Should potentially serve fixFirstOrderRecurrence()? | |
8979 | What's left as 'else' here, pointer inductions? Worth a TODO to model their backedges as well? | |
8982 | dyn_cast may return null; use cast<>? | |
8984 | Comments in VPRecipeBuilder.h that PhisToFix handles reduction phi's only should be updated, to include FOR phi's as well: /// Cross-iteration **reduction** phis for which we need to add the incoming value /// from the backedge after all recipes have been created. SmallVector<VPWidenPHIRecipe *, 4> PhisToFix; /// Add the incoming values from the backedge to **reduction** cross-iteration /// phis. void fixHeaderPhis(); Comments in VPlan.h about the backedge of VPWidenPHIRecipe should be updated as well: /// For **reduction** PHIs, RdxDesc must point to the corresponding recurrence /// descriptor, the start value is the first operand of the recipe and the /// incoming value from the backedge is the second operand. /// Returns the incoming value from the loop backedge, if it is a **reduction**. VPValue *getBackedgeValue() |
Addressed comments, thanks!
I also updated fixFirstOrderRecurrence to use the VPValues for the start and backedge values.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
4761–4765 | (Unrelated to this patch, but setting the loop's upper bound in advance to unsigned LastPartForNewPhi = IsOrdered ? 1 : State.UF; seems clearer than breaking inside the loop. Here and below.) | |
4775 | Instead of asking if (RdxDesc) here, move the following block earlier to after FOR(P)'s return; i.e., have the "if (RdxDesc || FOR(P))" fully handle both? This will also allow to keep the definitions of ScalarPHI and VecTy inside "if (RdxDesc || FOR(P))". | |
8968 | Add a VPWidenPHIRecipe(Phi, *StartV) constructor? (If not split into VPWidenReductionPHIRecipe, VPWidenFORPHIRecipe, VPWidenPointerIVPHIRecipe.) |
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
4761–4765 | Done in 7f369819774d | |
4775 | Sounds good, I moved the code up. | |
8968 | I added the new constructor. I'm planning to split them up as discussed at the other phi related patches as follow ups. |
This looks good to me, thanks!
Adding a couple of final nits.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
4293 | Better place this definition of Phi after the following comment, right before it is needed for defining Start? | |
4751–4752 | Better keep this definition of IsOrdered inside the if (RdxDesc || Legal->isFirstOrderRecurrence(P)) { below, right before it is used to define LastPartForNewPhi? |
Better place this definition of Phi after the following comment, right before it is needed for defining Start?