[VPlan] Introduce scalar loop header in plan, remove VPLiveOut.

[fhahn]

Update VPlan to include the scalar loop header. This allows retiring VPLiveOut, as the remaining live-outs can now be handled by adding operands to the wrapped phis in the scalar loop header.

Note that the current version only includes the scalar loop header, no other loop blocks and also does not wrap it in a region block.

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

Update VPlan to include the scalar loop header. This allows retiring VPLiveOut, as the remaining live-outs can now be handled by adding operands to the wrapped phis in the scalar loop header.

Note that the current version only includes the scalar loop header, no other loop blocks and also does not wrap it in a region block. This can either be included in this PR or in follow-ups as needed.

---

Full diff: https://github.com/llvm/llvm-project/pull/109975.diff

8 Files Affected:

• (modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+8-5)
• (modified) llvm/lib/Transforms/Vectorize/VPlan.cpp (+15-23)
• (modified) llvm/lib/Transforms/Vectorize/VPlan.h (-53)
• (modified) llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp (+4-30)
• (modified) llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp (+1-1)
• (modified) llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp (+6-6)
• (modified) llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp (-14)
• (modified) llvm/test/Transforms/LoopVectorize/vplan-sink-scalars-and-merge.ll (+18)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 5e4f33c55610f1..4305750eda26fc 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -2956,10 +2956,6 @@ void InnerLoopVectorizer::fixVectorizedLoop(VPTransformState &State,
                    IVEndValues[Entry.first], LoopMiddleBlock, Plan, State);
   }
 
-  // Fix live-out phis not already fixed earlier.
-  for (const auto &KV : Plan.getLiveOuts())
-    KV.second->fixPhi(Plan, State);
-
   for (Instruction *PI : PredicatedInstructions)
     sinkScalarOperands(&*PI);
 
@@ -8816,7 +8812,14 @@ static void addLiveOutsForFirstOrderRecurrences(
         VPInstruction::ResumePhi, {Resume, FOR->getStartValue()}, {},
         "scalar.recur.init");
     auto *FORPhi = cast<PHINode>(FOR->getUnderlyingInstr());
-    Plan.addLiveOut(FORPhi, ResumePhiRecipe);
+    for (VPRecipeBase &R :
+         *cast<VPIRBasicBlock>(ScalarPHVPBB->getSingleSuccessor())) {
+      auto *IRI = cast<VPIRInstruction>(&R);
+      if (&IRI->getInstruction() == FORPhi) {
+        IRI->addOperand(ResumePhiRecipe);
+        break;
+      }
+    }
 
     // Now update VPIRInstructions modeling LCSSA phis in the exit block.
     // Extract the penultimate value of the recurrence and use it as operand for
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.cpp b/llvm/lib/Transforms/Vectorize/VPlan.cpp
index 5e4d487261c6f0..e77fd8c5dca840 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlan.cpp
@@ -457,10 +457,17 @@ void VPIRBasicBlock::execute(VPTransformState *State) {
   State->Builder.SetInsertPoint(getIRBasicBlock()->getTerminator());
   executeRecipes(State, getIRBasicBlock());
   if (getSingleSuccessor()) {
-    assert(isa<UnreachableInst>(getIRBasicBlock()->getTerminator()));
-    auto *Br = State->Builder.CreateBr(getIRBasicBlock());
-    Br->setOperand(0, nullptr);
-    getIRBasicBlock()->getTerminator()->eraseFromParent();
+    auto *SuccVPIRBB = dyn_cast<VPIRBasicBlock>(getSingleSuccessor());
+    if (SuccVPIRBB && SuccVPIRBB->getIRBasicBlock() ==
+                          getIRBasicBlock()->getSingleSuccessor()) {
+      cast<BranchInst>(getIRBasicBlock()->getTerminator())
+          ->setOperand(0, nullptr);
+    } else {
+      assert(isa<UnreachableInst>(getIRBasicBlock()->getTerminator()));
+      auto *Br = State->Builder.CreateBr(getIRBasicBlock());
+      Br->setOperand(0, nullptr);
+      getIRBasicBlock()->getTerminator()->eraseFromParent();
+    }
   }
 
   for (VPBlockBase *PredVPBlock : getHierarchicalPredecessors()) {
@@ -844,10 +851,6 @@ void VPRegionBlock::print(raw_ostream &O, const Twine &Indent,
 #endif
 
 VPlan::~VPlan() {
-  for (auto &KV : LiveOuts)
-    delete KV.second;
-  LiveOuts.clear();
-
   if (Entry) {
     VPValue DummyValue;
     for (VPBlockBase *Block : vp_depth_first_shallow(Entry))
@@ -902,6 +905,8 @@ VPlanPtr VPlan::createInitialVPlan(Type *InductionTy,
   VPBlockUtils::insertBlockAfter(MiddleVPBB, TopRegion);
 
   VPBasicBlock *ScalarPH = new VPBasicBlock("scalar.ph");
+  VPBasicBlock *ScalarHeader = createVPIRBasicBlockFor(TheLoop->getHeader());
+  VPBlockUtils::connectBlocks(ScalarPH, ScalarHeader);
   if (!RequiresScalarEpilogueCheck) {
     VPBlockUtils::connectBlocks(MiddleVPBB, ScalarPH);
     return Plan;
@@ -1051,6 +1056,8 @@ void VPlan::execute(VPTransformState *State) {
   BrInst->insertBefore(MiddleBB->getTerminator());
   MiddleBB->getTerminator()->eraseFromParent();
   State->CFG.DTU.applyUpdates({{DominatorTree::Delete, MiddleBB, ScalarPh}});
+  State->CFG.DTU.applyUpdates(
+      {{DominatorTree::Delete, ScalarPh, ScalarPh->getSingleSuccessor()}});
 
   // Generate code in the loop pre-header and body.
   for (VPBlockBase *Block : vp_depth_first_shallow(Entry))
@@ -1169,12 +1176,6 @@ void VPlan::print(raw_ostream &O) const {
     Block->print(O, "", SlotTracker);
   }
 
-  if (!LiveOuts.empty())
-    O << "\n";
-  for (const auto &KV : LiveOuts) {
-    KV.second->print(O, SlotTracker);
-  }
-
   O << "}\n";
 }
 
@@ -1211,11 +1212,6 @@ LLVM_DUMP_METHOD
 void VPlan::dump() const { print(dbgs()); }
 #endif
 
-void VPlan::addLiveOut(PHINode *PN, VPValue *V) {
-  assert(LiveOuts.count(PN) == 0 && "an exit value for PN already exists");
-  LiveOuts.insert({PN, new VPLiveOut(PN, V)});
-}
-
 static void remapOperands(VPBlockBase *Entry, VPBlockBase *NewEntry,
                           DenseMap<VPValue *, VPValue *> &Old2NewVPValues) {
   // Update the operands of all cloned recipes starting at NewEntry. This
@@ -1283,10 +1279,6 @@ VPlan *VPlan::duplicate() {
   remapOperands(Preheader, NewPreheader, Old2NewVPValues);
   remapOperands(Entry, NewEntry, Old2NewVPValues);
 
-  // Clone live-outs.
-  for (const auto &[_, LO] : LiveOuts)
-    NewPlan->addLiveOut(LO->getPhi(), Old2NewVPValues[LO->getOperand(0)]);
-
   // Initialize remaining fields of cloned VPlan.
   NewPlan->VFs = VFs;
   NewPlan->UFs = UFs;
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index c886a39aec76e5..e2caf3dcdfd4af 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -671,48 +671,6 @@ class VPBlockBase {
   virtual VPBlockBase *clone() = 0;
 };
 
-/// A value that is used outside the VPlan. The operand of the user needs to be
-/// added to the associated phi node. The incoming block from VPlan is
-/// determined by where the VPValue is defined: if it is defined by a recipe
-/// outside a region, its parent block is used, otherwise the middle block is
-/// used.
-class VPLiveOut : public VPUser {
-  PHINode *Phi;
-
-public:
-  VPLiveOut(PHINode *Phi, VPValue *Op)
-      : VPUser({Op}, VPUser::VPUserID::LiveOut), Phi(Phi) {}
-
-  static inline bool classof(const VPUser *U) {
-    return U->getVPUserID() == VPUser::VPUserID::LiveOut;
-  }
-
-  /// Fix the wrapped phi node. This means adding an incoming value to exit
-  /// block phi's from the vector loop via middle block (values from scalar loop
-  /// already reach these phi's), and updating the value to scalar header phi's
-  /// from the scalar preheader.
-  void fixPhi(VPlan &Plan, VPTransformState &State);
-
-  /// Returns true if the VPLiveOut uses scalars of operand \p Op.
-  bool usesScalars(const VPValue *Op) const override {
-    assert(is_contained(operands(), Op) &&
-           "Op must be an operand of the recipe");
-    return true;
-  }
-
-  PHINode *getPhi() const { return Phi; }
-
-  /// Live-outs are marked as only using the first part during the transition
-  /// to unrolling directly on VPlan.
-  /// TODO: Remove after unroller transition.
-  bool onlyFirstPartUsed(const VPValue *Op) const override { return true; }
-
-#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-  /// Print the VPLiveOut to \p O.
-  void print(raw_ostream &O, VPSlotTracker &SlotTracker) const;
-#endif
-};
-
 /// Struct to hold various analysis needed for cost computations.
 struct VPCostContext {
   const TargetTransformInfo &TTI;
@@ -3454,11 +3412,6 @@ class VPlan {
   /// definitions are VPValues that hold a pointer to their underlying IR.
   SmallVector<VPValue *, 16> VPLiveInsToFree;
 
-  /// Values used outside the plan. It contains live-outs that need fixing. Any
-  /// live-out that is fixed outside VPlan needs to be removed. The remaining
-  /// live-outs are fixed via VPLiveOut::fixPhi.
-  MapVector<PHINode *, VPLiveOut *> LiveOuts;
-
   /// Mapping from SCEVs to the VPValues representing their expansions.
   /// NOTE: This mapping is temporary and will be removed once all users have
   /// been modeled in VPlan directly.
@@ -3638,12 +3591,6 @@ class VPlan {
     return cast<VPCanonicalIVPHIRecipe>(&*EntryVPBB->begin());
   }
 
-  void addLiveOut(PHINode *PN, VPValue *V);
-
-  const MapVector<PHINode *, VPLiveOut *> &getLiveOuts() const {
-    return LiveOuts;
-  }
-
   VPValue *getSCEVExpansion(const SCEV *S) const {
     return SCEVToExpansion.lookup(S);
   }
diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
index f33293e65010f9..2ba659fb850852 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -197,35 +197,6 @@ bool VPRecipeBase::mayHaveSideEffects() const {
   }
 }
 
-void VPLiveOut::fixPhi(VPlan &Plan, VPTransformState &State) {
-  VPValue *ExitValue = getOperand(0);
-  VPBasicBlock *MiddleVPBB =
-      cast<VPBasicBlock>(Plan.getVectorLoopRegion()->getSingleSuccessor());
-  VPRecipeBase *ExitingRecipe = ExitValue->getDefiningRecipe();
-  auto *ExitingVPBB = ExitingRecipe ? ExitingRecipe->getParent() : nullptr;
-  // Values leaving the vector loop reach live out phi's in the exiting block
-  // via middle block.
-  auto *PredVPBB = !ExitingVPBB || ExitingVPBB->getEnclosingLoopRegion()
-                       ? MiddleVPBB
-                       : ExitingVPBB;
-  BasicBlock *PredBB = State.CFG.VPBB2IRBB[PredVPBB];
-  Value *V = State.get(ExitValue, VPIteration(0, 0));
-  if (Phi->getBasicBlockIndex(PredBB) != -1)
-    Phi->setIncomingValueForBlock(PredBB, V);
-  else
-    Phi->addIncoming(V, PredBB);
-}
-
-#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-void VPLiveOut::print(raw_ostream &O, VPSlotTracker &SlotTracker) const {
-  O << "Live-out ";
-  getPhi()->printAsOperand(O);
-  O << " = ";
-  getOperand(0)->printAsOperand(O, SlotTracker);
-  O << "\n";
-}
-#endif
-
 void VPRecipeBase::insertBefore(VPRecipeBase *InsertPos) {
   assert(!Parent && "Recipe already in some VPBasicBlock");
   assert(InsertPos->getParent() &&
@@ -867,7 +838,10 @@ void VPIRInstruction::execute(VPTransformState &State) {
     State.Builder.SetInsertPoint(PredBB, PredBB->getFirstNonPHIIt());
     Value *V = State.get(ExitValue, VPIteration(0, Lane));
     auto *Phi = cast<PHINode>(&I);
-    Phi->addIncoming(V, PredBB);
+    if (Phi->getBasicBlockIndex(PredBB) == -1)
+      Phi->addIncoming(V, PredBB);
+    else
+      Phi->setIncomingValueForBlock(PredBB, V);
   }
 
   // Advance the insert point after the wrapped IR instruction. This allows
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
index 3b37a1ec9560ee..00f12bf915e0d8 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -378,7 +378,7 @@ static bool mergeBlocksIntoPredecessors(VPlan &Plan) {
     // Don't fold the exit block of the Plan into its single predecessor for
     // now.
     // TODO: Remove restriction once more of the skeleton is modeled in VPlan.
-    if (VPBB->getNumSuccessors() == 0 && !VPBB->getParent())
+    if (!VPBB->getParent())
       continue;
     auto *PredVPBB =
         dyn_cast_or_null<VPBasicBlock>(VPBB->getSinglePredecessor());
diff --git a/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp b/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp
index 4907d3f0397274..17a4cbd36bd30d 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp
@@ -265,6 +265,7 @@ void UnrollState::unrollRecipeByUF(VPRecipeBase &R) {
 
   if (auto *VPI = dyn_cast<VPInstruction>(&R)) {
     VPValue *Op0, *Op1;
+
     if (match(VPI, m_VPInstruction<VPInstruction::ExtractFromEnd>(
                        m_VPValue(Op0), m_VPValue(Op1)))) {
       VPI->setOperand(1, getValueForPart(Op1, UF - 1));
@@ -281,6 +282,11 @@ void UnrollState::unrollRecipeByUF(VPRecipeBase &R) {
       }
       return;
     }
+    if (match(VPI, m_VPInstruction<VPInstruction::ResumePhi>(m_VPValue(Op0),
+                                                             m_VPValue(Op1)))) {
+      addUniformForAllParts(VPI);
+      return;
+    }
 
     if (vputils::onlyFirstPartUsed(VPI)) {
       addUniformForAllParts(VPI);
@@ -467,11 +473,5 @@ void VPlanTransforms::unrollByUF(VPlan &Plan, unsigned UF, LLVMContext &Ctx) {
     Part++;
   }
 
-  // Remap the operand of live-outs to the last part.
-  for (const auto &[_, LO] : Plan.getLiveOuts()) {
-    VPValue *In = Unroller.getValueForPart(LO->getOperand(0), UF - 1);
-    LO->setOperand(0, In);
-  }
-
   VPlanTransforms::removeDeadRecipes(Plan);
 }
diff --git a/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp b/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
index 99bc4c38a3c3cd..4badf295092827 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
@@ -244,14 +244,6 @@ bool VPlanVerifier::verifyVPBasicBlock(const VPBasicBlock *VPBB) {
     return false;
   }
 
-  VPBlockBase *MiddleBB =
-      IRBB->getPlan()->getVectorLoopRegion()->getSingleSuccessor();
-  if (IRBB != IRBB->getPlan()->getPreheader() &&
-      IRBB->getSinglePredecessor() != MiddleBB) {
-    errs() << "VPIRBasicBlock can only be used as pre-header or a successor of "
-              "middle-block at the moment!\n";
-    return false;
-  }
   return true;
 }
 
@@ -416,12 +408,6 @@ bool VPlanVerifier::verify(const VPlan &Plan) {
     return false;
   }
 
-  for (const auto &KV : Plan.getLiveOuts())
-    if (KV.second->getNumOperands() != 1) {
-      errs() << "live outs must have a single operand\n";
-      return false;
-    }
-
   return true;
 }
 
diff --git a/llvm/test/Transforms/LoopVectorize/vplan-sink-scalars-and-merge.ll b/llvm/test/Transforms/LoopVectorize/vplan-sink-scalars-and-merge.ll
index 0f3cd9d4ca4d61..2dddf766cb9cda 100644
--- a/llvm/test/Transforms/LoopVectorize/vplan-sink-scalars-and-merge.ll
+++ b/llvm/test/Transforms/LoopVectorize/vplan-sink-scalars-and-merge.ll
@@ -1077,6 +1077,17 @@ define void @merge_with_dead_gep_between_regions(i32 %n, ptr noalias %src, ptr n
 ; CHECK-NEXT: No successors
 ; CHECK-EMPTY:
 ; CHECK-NEXT: scalar.ph
+; CHECK-NEXT: Successor(s): ir-bb<loop>
+; CHECK-EMPTY:
+; CHECK-NEXT: ir-bb<loop>:
+; CHECK-NEXT:   IR   %iv = phi i32 [ %n, %entry ], [ %iv.next, %loop ]
+; CHECK-NEXT:   IR   %iv.next = add nsw i32 %iv, -1
+; CHECK-NEXT:   IR   %gep.src = getelementptr inbounds i32, ptr %src, i32 %iv
+; CHECK-NEXT:   IR   %l = load i32, ptr %gep.src, align 16
+; CHECK-NEXT:   IR   %dead_gep = getelementptr inbounds i32, ptr %dst, i64 1
+; CHECK-NEXT:   IR   %gep.dst = getelementptr inbounds i32, ptr %dst, i32 %iv
+; CHECK-NEXT:   IR   store i32 %l, ptr %gep.dst, align 16
+; CHECK-NEXT:   IR   %ec = icmp eq i32 %iv.next, 0
 ; CHECK-NEXT: No successors
 ; CHECK-NEXT: }
 ;
@@ -1156,6 +1167,13 @@ define void @ptr_induction_remove_dead_recipe(ptr %start, ptr %end) {
 ; CHECK-NEXT: No successors
 ; CHECK-EMPTY:
 ; CHECK-NEXT: scalar.ph:
+; CHECK-NEXT: Successor(s): ir-bb<loop.header>
+; CHECK-EMPTY:
+; CHECK-NEXT: ir-bb<loop.header>:
+; CHECK-NEXT:   IR   %ptr.iv = phi ptr [ %start, %entry ], [ %ptr.iv.next, %loop.latch ]
+; CHECK-NEXT:   IR   %ptr.iv.next = getelementptr inbounds i8, ptr %ptr.iv, i64 -1
+; CHECK-NEXT:   IR   %l = load i8, ptr %ptr.iv.next, align 1
+; CHECK-NEXT:   IR   %c.1 = icmp eq i8 %l, 0
 ; CHECK-NEXT: No successors
 ; CHECK-NEXT: }
 ;

[david-arm]

I just noticed this patch today. I have a patch to enable vectorisation of more early exit loops that require adding live outs from the early exiting block - #88385. I hadn't realised that VPLiveOuts were going to be removed from the vplan.

@fhahn If possible, can you point me at the patch or code that shows how we model outside uses of loop values in the exit block? I will need to update PR #88385 to ensure we generate the correct final value in the early exit block.

[david-arm]

Also, is there a document somewhere that highlights (even at a high level) some of the refactoring work like this that is planned in the next few months? Just because I'm also actively working in this area. Or if not, is it possible to tag me as a reviewer or subscriber for other patches in this area? Thanks!

[fhahn]

I just noticed this patch today. I have a patch to enable vectorisation of more early exit loops that require adding live outs from the early exiting block - #88385. I hadn't realised that VPLiveOuts were going to be removed from the vplan.

@fhahn If possible, can you point me at the patch or code that shows how we model outside uses of loop values in the exit block? I will need to update PR #88385 to ensure we generate the correct final value in the early exit block.

I think we should already have most of the pieces needed to model the exit values in the early exit the same way as for the regular exits after introducing VPIRBasicBlocks and VPIRInstructions. #100735 uses those for the regular exit phis.

It should be possible create a VPIRBasicBlock for the early exit block in a similar fashion and add operands to the VPIRInstructions of the phis that need updating.

A few small changes may be needed to support VPIRBBs and VPIRInst phis with multiple predecessors, but hopefully something like 3cfe25b should be enough.

[fhahn]

Or if not, is it possible to tag me as a reviewer or subscriber for other patches in this area? Thanks!

Probably the best way would be to join the subscriber team for the vectorizer label: https://github.com/orgs/llvm/teams/pr-subscribers-vectorizers

[david-arm]

I just noticed this patch today. I have a patch to enable vectorisation of more early exit loops that require adding live outs from the early exiting block - #88385. I hadn't realised that VPLiveOuts were going to be removed from the vplan.
@fhahn If possible, can you point me at the patch or code that shows how we model outside uses of loop values in the exit block? I will need to update PR #88385 to ensure we generate the correct final value in the early exit block.

I think we should already have most of the pieces needed to model the exit values in the early exit the same way as for the regular exits after introducing VPIRBasicBlocks and VPIRInstructions. #100735 uses those for the regular exit phis.

It should be possible create a VPIRBasicBlock for the early exit block in a similar fashion and add operands to the VPIRInstructions of the phis that need updating.

A few small changes may be needed to support VPIRBBs and VPIRInst phis with multiple predecessors, but hopefully something like 3cfe25b should be enough.

OK thank you for the info - I'll take a look!

[david-arm]

Or if not, is it possible to tag me as a reviewer or subscriber for other patches in this area? Thanks!

Probably the best way would be to join the subscriber team for the vectorizer label: https://github.com/orgs/llvm/teams/pr-subscribers-vectorizers

Yeah, I remember when we first moved to github I added myself to that list and I didn't seem to get any notifications of vectoriser patches, despite it being pretty obvious that patches were being created. So I took the alternative approach of saving a link to a github search query that shows all open vectoriser patches. Maybe it has started working now?

[fhahn]

I just noticed this patch today. I have a patch to enable vectorisation of more early exit loops that require adding live outs from the early exiting block - #88385. I hadn't realised that VPLiveOuts were going to be removed from the vplan.
@fhahn If possible, can you point me at the patch or code that shows how we model outside uses of loop values in the exit block? I will need to update PR #88385 to ensure we generate the correct final value in the early exit block.

I think we should already have most of the pieces needed to model the exit values in the early exit the same way as for the regular exits after introducing VPIRBasicBlocks and VPIRInstructions. #100735 uses those for the regular exit phis.
It should be possible create a VPIRBasicBlock for the early exit block in a similar fashion and add operands to the VPIRInstructions of the phis that need updating.
A few small changes may be needed to support VPIRBBs and VPIRInst phis with multiple predecessors, but hopefully something like 3cfe25b should be enough.

OK thank you for the info - I'll take a look!

https://github.com/llvm/llvm-project/pull/109193/files#diff-c33c5be02bdbedc5499230934c8fc873429b771425f21c3f594acb6fae6339d7 should show how the VPlans would look like

[david-arm]

Hi @fhahn, so I tried changing #88385 to use the new way of dealing with live-outs and not rely upon VPLiveOut in a similar way to how we deal with normal exits. However, I came across a fundamental ordering constraint that needs dealing with. The problem occurs when the exit block from the latch is the same as the exit from the early exiting block. In this case the vectorised CFG would look something like this:

vector.loop:
  ...
  br i1, label %vector.early.exit, label %vector.loop.latch

vector.loop.latch:
  br i1, label %middle.block, label %vector.loop

vector.early.exit:
  ...
  br label %loop.exit

middle.block:
  ...
  br i1 %finished, label %loop.exit, ...

... original scalar loop sits here ...

VPIRBasicBlock(loop.exit):
  VPIRInstruction(%tea = phi i32 [ ..., %loop ], [ ..., %loop.latch ] (extra operands ...))

Using the example implementation you mentioned in 3cfe25b (thank you for this!) the order of the predecessors in VPIRBasicBlock(loop.exit) by definition has to match the same order in which the extra operands were added to VPIRInstruction(%tea) when collecting/adding users in the exit block during VPlan creation. However, VPlan::execute breaks this assumption immediately when rewiring the middle block from a VPBasicBlock to a VPIRBasicBlock. In this case I have two choices that I know of:

1. In replaceVPBBWithIRVPBB ensure that the middle block stays in the same position in the predecessor list of any successor. I imagine it's possible, but it does feel fragile.
2. Add a map to VPIRInstruction that maps the additional operand index with the incoming block so that when rewiring the middle block we can ensure the map stays correct. This removes any fragile ordering dependency. Then I'd adapt VPIRInstruction::execute in 3cfe25b to get the incoming block by looking the index up in the map.

I'll keep playing around with this a bit more and see what works best!

[david-arm]

Hi @fhahn, whilst the PR looks fine in isolation I'd rather it not land until we've had a chance to discuss its impact on PR #88385 if that's ok? Thanks!

[fhahn]

Sure. Curious if you hit any limitations with VPIRInstruction that prevent it from replacing VPLiveOut?

[david-arm]

Sure. Curious if you hit any limitations with VPIRInstruction that prevent it from replacing VPLiveOut?

Yeah, so in my comment earlier last week I highlighted the problem of ordering constraints. If the same exit block is used by both the latch and the early exit, then the phi in the exit block should have an incoming value for both of the exiting blocks. If I add operands to the VPIRInstruction wrapper - one for each exiting block - then in the VPIRInstruction::execute code using a patch you sketched out in 3cfe25b we introduce a requirement that the order of the extra operands must match the order of the predecessors. replaceVPBBWithIRVPBB breaks this by reordering them, hence I put up a patch yesterday to preempt this - #111514. Perhaps it's fine to introduce this ordering constraint, but I wonder if this makes the code fragile and prone to breaking every time we do some CFG optimisation? Secondly, the phi may or may not reference an induction variable in the loop, which is handled separately to VPIRInstruction::execute. However, you still need to add some sort of dummy operand because the number of operands must match the number of predecessors. I have hacked this downstream by adding a static 'null' VPValue marker that we can skip over the operand in VPIRInstruction::execute.

Another approach would be to introduce a new class derived from VPIRInstruction, i.e. VPIRPhiInstruction that maintains a mapping between the extra operand and the incoming block. I haven't tried this second approach - this is potentially more flexible, but it is moving a bit further away from having a simple IR wrapper and is moving back towards something like VPLiveOut.

[kstoimenov]

@fhahn you might have introduced a memory leak in this patch detected by Sanitizer bots: https://lab.llvm.org/buildbot/#/builders/169/builds/4844/steps/10/logs/stdio

Please take a look.

[fhahn]

Should be fixed in 4bc8bde0bc4b0dbace5545de5792be2138e9cf64, thanks!

[kstoimenov]

Looks like there is still one more leak left after your fix: https://lab.llvm.org/buildbot/#/builders/169/builds/4871/steps/10/logs/stdio

[kstoimenov]

FYI, @vitalybuka pushed this fix: 80b9f07