[PATCH] D150398: [VPlan] Model branch cond to enter scalar epilogue in VPlan.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3714
   // in the exit block, so update the builder.
   State.Builder.SetInsertPoint(State.CFG.ExitBB->getFirstNonPHI());
   for (const auto &KV : Plan.getLiveOuts())
----------------
Ayal wrote:
> (Independent of this patch): builder insertion point should better be used for recipe execution only (and then unset), rather than passing MiddleBlock with it for live-out fixing, which should arguably take care of themselves.
This can be removed after this patch lands (more specifically after the insert point is set in VPBasicBlock::execute), but reorders some extracts, so would be better as a follow-up I think.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3716
   for (const auto &KV : Plan.getLiveOuts())
-    KV.second->fixPhi(Plan, State);
+    KV.second->execute(Plan, State);
 
----------------
Ayal wrote:
> LiveOuts "fix" existing IR Instructions, rather than "execute" - generate new IR Instructions.
(nit: LiveOuts should be a small vector of VPLiveOutPhi's in this case.)



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3858
       assert(!Cost->requiresScalarEpilogue(VF));
-      PHINode *LCSSAPhi = LiveOut->getPhi();
+      PHINode *LCSSAPhi = cast<VPLiveOutPhi>(LiveOut)->getPhi();
       LCSSAPhi->addIncoming(ExtractForPhiUsedOutsideLoop, LoopMiddleBlock);
----------------
Ayal wrote:
> (nit: LiveOuts should be a small vector of VPLiveOutPhi's in this case.)
> 
> (Independent of this patch): Above should be handled by recipe(s) and below by fixPhi().
> (nit: LiveOuts should be a small vector of VPLiveOutPhi's in this case.)

Code got removed.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8811
 static void addUsersInExitBlock(VPBasicBlock *HeaderVPBB,
                                 VPBasicBlock *MiddleVPBB, Loop *OrigLoop,
                                 VPlan &Plan) {
----------------
Ayal wrote:
> While we're here, MiddleVPBB is unused (relates to patch D123537 rather than this patch).
Removed in 9ee4a740e326, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9091
+                          ScalarLatchTerm->getDebugLoc());
+    VPBB->appendRecipe(Cmp);
+    Plan->addLiveOut(nullptr, new VPLiveOutBr(Cmp));
----------------
Ayal wrote:
> This is a first population of MiddleVPBB with recipes, following its introduction in D123457, right?
> 
> Presumably, all instructions of MiddleBlock should eventually be generated by VPlan recipes/VPInstructions.
> 
> The conditional branch whose condition will be backpatched to Cmp is generated by createVectorLoopSkeleton() as part of the vectorization process, rather than being an **original Instruction** that can be wrapped with a VPLiveOut - like the LCSSA Phi's of the Exit block.
> Could this conditional branch terminating MiddleVPBB be generated as a recipe too (w/o any further live-out users), along with moving the generation of its compare from skeleton to VPlan, rather than fixing it via LiveOutBr? I.e., extend/have a forward BranchOnCond as mentioned when introduced in D126618 to represent loop/backward branches, and similar to BranchOnMask. Following VPlan's approach of rewiring branches when visiting their successors, the two successors of MiddleBlock (Exit block and ScalarPreHeader/VectorEpilogTripCountCheck) could be wrapped in VPBB's, similar to the current wrapping of MiddleVPBB (only) to rewire the vector loop latch branch. These two successors should reuse pre-built IRBB's, similar to MiddleVPBB, which in turn can have VPlan generate its IRBB instead of createVectorLoopSkeleton().
> 
> Other skeletal parts, including bypass basic blocks between Preheader and VecPreheader, will also need similar forward out-of-loop conditional branch recipes, when migrated to VPlan.
> 
> WDYT?
Updated to use `BranchOnCond`; as `BasicBlock` is a subclass of `Value` it is possible to wrap them in live-in VPValues


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9092
+    VPBB->appendRecipe(Cmp);
+    Plan->addLiveOut(nullptr, new VPLiveOutBr(Cmp));
+  }
----------------
Ayal wrote:
> Note that this accommodates only a single VPLiveOutBr due to insertion with null key into a unique map. Would be good to get rid of that map anyhow.
Code got removed.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:358
     cast<BranchInst>(ExitingBB->getTerminator())->setSuccessor(0, NewBB);
+    State->Builder.SetInsertPoint(NewBB->getTerminator());
   } else if (PrevVPBB && /* A */
----------------
Ayal wrote:
> Ayal wrote:
> > Retain comment?
> Worth a comment, setting builder to execute recipes at (the end of) "this" MiddleVPBB?
We need to set the insert point in both cases, sunk outside if.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:358
-    // The Exit block of a loop is always set to be successor 0 of the Exiting
-    // block.
     cast<BranchInst>(ExitingBB->getTerminator())->setSuccessor(0, NewBB);
----------------
fhahn wrote:
> Ayal wrote:
> > Ayal wrote:
> > > Retain comment?
> > Worth a comment, setting builder to execute recipes at (the end of) "this" MiddleVPBB?
> We need to set the insert point in both cases, sunk outside if.
Retained, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:682
   State->CFG.PrevVPBB = nullptr;
   State->CFG.ExitBB = State->CFG.PrevBB->getSingleSuccessor();
   BasicBlock *VectorPreHeader = State->CFG.PrevBB;
----------------
Ayal wrote:
> (Independent of this patch:) Note that this may be a confusing choice of names: CFG.ExitBB is set to "MiddleBlock", which is the **new** Exit-from-loop block, as opposed to the original Exit-from-loop-block(s), aka "Exit".
Yes, should be updated to MiddleBlock for now? I think once the current patch is in, we could also handle exit block and scalar.ph creation in VPlan, to remove the need to create branches up front.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:169
   Phi->addIncoming(State.get(ExitValue, VPIteration(State.UF - 1, Lane)),
                    State.Builder.GetInsertBlock());
 }
----------------
Ayal wrote:
> (Independent of this patch) Better to retrieve
> `BasicBlock *FromBB = State->CFG.VPBB2IRBB[FromVPBB];`
> than to rely on State.Builder to be set to MiddleBB,
> where the LiveOut is initialized with FromVPBB = MiddleVPBB - conceptually as another operand, VPBB2IRBB[] analogous to State.get().
Updated here ada2a455fca4 to use the middle VPBB from VPlan for now, as this is what should be used implicitly


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:267
+    if (Part != 0)
+      break;
+    Value *IV = State.get(getOperand(0), {Part, 0});
----------------
Ayal wrote:
> This ICmpEQ is designed to be placed and executed out-of-loop, assert Part is zero?
The modeling here will need future refinement. At the moment all recipes are executed UF times I think. As discussed at a different patch, I am working on patches to allow explicit cloning of recipes to eliminate the implicit dependence on UF.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:272
+        CmpInst::Create(Instruction::ICmp, CmpInst::ICMP_EQ, IV, TC, "",
+                        &*Builder.GetInsertBlock()->getTerminator());
+    Cmp->setDebugLoc(DL);
----------------
Ayal wrote:
> Builder used to indicate insertion block rather than point? Builder.CreateICmpEQ()?
Split off refactoring to use Builder in old skeleton construction to avoid unrelated changes in the diff: the builder will simplify ICmps with constant operands D158332. Should be able to perform this simplification in VPlan directly eventually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150398