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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal 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())
----------------
(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.


================
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);
 
----------------
LiveOuts "fix" existing IR Instructions, rather than "execute" - generate new IR Instructions.


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9075
+  // 3) Otherwise, construct a runtime check.
+  if (LoopVectorizationPlanner::getDecisionAndClampRange(
+          [this](ElementCount VF) { return !CM.requiresScalarEpilogue(VF); },
----------------
This compare is mandatory (where needed) - should it be introduced as part of building the initial VPlan rather than here as a VPlan2VPlan transformation? In any case, probably better to outline out of the excessive tryToBuildVPlanWithVPRecipes().


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9091
+                          ScalarLatchTerm->getDebugLoc());
+    VPBB->appendRecipe(Cmp);
+    Plan->addLiveOut(nullptr, new VPLiveOutBr(Cmp));
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9092
+    VPBB->appendRecipe(Cmp);
+    Plan->addLiveOut(nullptr, new VPLiveOutBr(Cmp));
+  }
----------------
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.


================
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);
----------------
Retain comment?


================
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:
> Retain comment?
Worth a comment, setting builder to execute recipes at (the end of) "this" MiddleVPBB?


================
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;
----------------
(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".


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:169
   Phi->addIncoming(State.get(ExitValue, VPIteration(State.UF - 1, Lane)),
                    State.Builder.GetInsertBlock());
 }
----------------
(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().


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:267
+    if (Part != 0)
+      break;
+    Value *IV = State.get(getOperand(0), {Part, 0});
----------------
This ICmpEQ is designed to be placed and executed out-of-loop, assert Part is zero?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:272
+        CmpInst::Create(Instruction::ICmp, CmpInst::ICMP_EQ, IV, TC, "",
+                        &*Builder.GetInsertBlock()->getTerminator());
+    Cmp->setDebugLoc(DL);
----------------
Builder used to indicate insertion block rather than point? Builder.CreateICmpEQ()?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150398