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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn 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,
----------------
Ayal wrote:
> Above comment deserves an update?
Yes, I updated it in the latest version. Should only generate code for first-order recurrence phis and pointer inductions.


================
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);
----------------
Ayal wrote:
> `dyn_cast<VPReductionPHIRecipe>(PhiR)` or directly `dyn_cast<VPReductionPHIRecipe>(&R)`, instead of going from PhiR to its VPValue base class and back via getDef()?
Done! Needed to add a `classof` implementation for `VPWidenPHIRecipe` to `VPReductionPHIRecipe`, to resolve multiple available overloads.


================
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);
----------------
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`.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1166
+    } else {
+      IRBuilderBase::InsertPointGuard IPBuilder(Builder);
+      Builder.SetInsertPoint(State.CFG.VectorPreHeader->getTerminator());
----------------
Ayal wrote:
> 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?
> Sounds reasonable, perhaps as a follow-up?

+1, I'll look into modeling & generating the code in the pre-header as a follow-up patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104989