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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:508
   /// variable canonicalization. It supports both VF = 1 for unrolled loops and
   /// arbitrary length vectors.
+  void widenPHIInstruction(Instruction *PN, VPWidenPHIRecipe *PhiR,
----------------
Above comment deserves an update?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4135
     auto *OrigPhi = cast<PHINode>(PhiR->getUnderlyingValue());
-    if (PhiR->getRecurrenceDescriptor()) {
-      fixReduction(PhiR, State);
+    if (auto *ReductionPhi = dyn_cast<VPReductionPHIRecipe>(PhiR->getDef())) {
+      fixReduction(ReductionPhi, State);
----------------
`dyn_cast<VPReductionPHIRecipe>(PhiR)` or directly `dyn_cast<VPReductionPHIRecipe>(&R)`, instead of going from PhiR to its VPValue base class and back via getDef()?


================
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:
> > 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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1166
+    } else {
+      IRBuilderBase::InsertPointGuard IPBuilder(Builder);
+      Builder.SetInsertPoint(State.CFG.VectorPreHeader->getTerminator());
----------------
Code that VPlan is to generate inside the vector preheader, should ideally (/arguably?) be represented by recipes that reside inside a VPBasicBlock which represents this preheader, thereby properly extending the scope of VPlan; rather than redirecting Builder's insertion point. Sounds reasonable, perhaps as a follow-up?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104989