[PATCH] D104989: [VPlan] Add VPReductionPHIRecipe (NFC).

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

Looks good to me, adding a few minor optional last comments.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8043
   State.CFG.PrevBB = ILV.createVectorizedLoopSkeleton();
+  State.CFG.VectorPreHeader = State.CFG.PrevBB;
   State.TripCount = ILV.getOrCreateTripCount(nullptr);
----------------
fhahn wrote:
> Ayal wrote:
> > fhahn wrote:
> > > Ayal wrote:
> > > > Are both PrevBB and VectorPreHeader needed?
> > > Not really, we can use LoopInfo to get the pre-header block. Updated the code.
> > > Are both PrevBB and VectorPreHeader needed?
> > 
> > Ahh, it indeed seems more logical to set `State.CFG.VectorPreHeader = ILV.createVectorizedLoopSkeleton();` here; and then have VPlan::execute() retrieve this VectorPreHeader and set/reset State.CFG.PrevBB, LastBB during code-gen of the vector loop's basic blocks.
> > 
> > But this issue of having to record the preheader in an additional State field, or look for it via LoopInfo, raises a more general thought regarding VPlan generating preheader code, noted below.
> Ok, I went back to having a separate field for the vector preheader. I kept the code to set both `VectorPreHeader` and `PrevBB` to the block returned by `createVectorizedLoopSkeleton`.
On second thought, considering that State.CFG.VectorPreHeader may be temporary, it may be simpler to set it locally (and temporarily) inside VPlan::execute() as mentioned below.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:765
 
-  BasicBlock *VectorPreHeaderBB = State->CFG.PrevBB;
+  BasicBlock *VectorPreHeaderBB = State->CFG.VectorPreHeader;
   BasicBlock *VectorHeaderBB = VectorPreHeaderBB->getSingleSuccessor();
----------------
Another option, to keep this change local, is to keep
`  BasicBlock *VectorPreHeaderBB = State->CFG.PrevBB;`
and set here
`  State->CFG.VectorPreHeader = VectorPreHeaderBB;`


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1124
+  // directly.
+  // TODO: Remove once all VPWidenPHIRecipe instances keep all relevant incoming
+  // values as VPValues.
----------------
This TODO and associated `if` block below can be removed for VPReductionPHIRecipe?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1149
+  BasicBlock *HeaderBB = State.CFG.PrevBB;
+  Loop *L = State.LI->getLoopFor(HeaderBB);
+  assert(L->getHeader() == HeaderBB &&
----------------
`L` used only in assert?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1054
+/// A recipe for handling first order recurrences and pointer inductions.
+/// For first-order recurrences, the start value is
 /// the first operand of the recipe and the incoming value from the backedge is
----------------
Mention that VPWidenPHIRecipe also serves as a base class for reduction phis?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104989