[VPlan] Retain exit conditions and edges in initial VPlan (NFC). #137709

fhahn · 2025-04-28T21:09:40Z

Update initial VPlan construction to include exit conditions and edges.

The loop region is now first constructed without entry/exiting. Those are set after inserting the region in the CFG, to preserve the original predecessor/successor order of blocks.

For now, all early exits are disconnected before forming the regions, but a follow-up will update uncountable exit handling to also happen here. This is required to enable VPlan predication and remove the dependence any IR BBs (#128420).

llvmbot · 2025-04-28T21:10:15Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-vectorizers

Author: Florian Hahn (fhahn)

Changes

Update initial VPlan construction to include exit conditions and edges.

For now, all early exits are disconnected before forming the regions, but a follow-up will update uncountable exit handling to also happen here. This is required to enable VPlan predication and remove the dependence any IR BBs (#128420).

This includes updates in a few places to use
replaceSuccessor/replacePredecessor to preserve the order of predecessors and successors, to reduce the need of fixing up phi operand orderings. This unfortunately required making them public, not sure if there's a

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

4 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+11-1)
(modified) llvm/lib/Transforms/Vectorize/VPlan.h (+1-2)
(modified) llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp (+38-40)
(modified) llvm/test/Transforms/LoopVectorize/vplan-printing-outer-loop.ll (+5-1)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 4684378687ef6..bd043ae5e06e8 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -9167,6 +9167,7 @@ void LoopVectorizationPlanner::buildVPlansWithVPRecipes(ElementCount MinVF,
 // loop.
 static void addCanonicalIVRecipes(VPlan &Plan, Type *IdxTy, bool HasNUW,
                                   DebugLoc DL) {
+  using namespace VPlanPatternMatch;
   Value *StartIdx = ConstantInt::get(IdxTy, 0);
   auto *StartV = Plan.getOrAddLiveIn(StartIdx);
 
@@ -9176,7 +9177,16 @@ static void addCanonicalIVRecipes(VPlan &Plan, Type *IdxTy, bool HasNUW,
   VPBasicBlock *Header = TopRegion->getEntryBasicBlock();
   Header->insert(CanonicalIVPHI, Header->begin());
 
-  VPBuilder Builder(TopRegion->getExitingBasicBlock());
+  VPBasicBlock *LatchVPBB = TopRegion->getExitingBasicBlock();
+  // We are about to replace the branch to exit the region. Remove the original
+  // BranchOnCond, if there is any.
+  // TODO: Move canonical IV and BranchOnCount introduction to initial skeleton
+  // creation.
+  if (!LatchVPBB->empty() &&
+      match(&LatchVPBB->back(), m_BranchOnCond(m_VPValue())))
+    LatchVPBB->getTerminator()->eraseFromParent();
+
+  VPBuilder Builder(LatchVPBB);
   // Add a VPInstruction to increment the scalar canonical IV by VF * UF.
   auto *CanonicalIVIncrement = Builder.createOverflowingOp(
       Instruction::Add, {CanonicalIVPHI, &Plan.getVFxUF()}, {HasNUW, false}, DL,
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index afad73bcd3501..5d81bcdc4a21e 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -117,6 +117,7 @@ class VPBlockBase {
     Predecessors.erase(Pos);
   }
 
+public:
   /// Remove \p Successor from the successors of this block.
   void removeSuccessor(VPBlockBase *Successor) {
     auto Pos = find(Successors, Successor);
@@ -129,8 +130,6 @@ class VPBlockBase {
   void replacePredecessor(VPBlockBase *Old, VPBlockBase *New) {
     auto I = find(Predecessors, Old);
     assert(I != Predecessors.end());
-    assert(Old->getParent() == New->getParent() &&
-           "replaced predecessor must have the same parent");
     *I = New;
   }
 
diff --git a/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp b/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp
index 58d63933fb724..af198bbf57df6 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp
@@ -112,6 +112,9 @@ VPBasicBlock *PlainCFGBuilder::getOrCreateVPBB(BasicBlock *BB) {
     return VPBB;
   }
 
+  if (!TheLoop->contains(BB))
+    return Plan->getExitBlock(BB);
+
   // Create new VPBB.
   StringRef Name = BB->getName();
   LLVM_DEBUG(dbgs() << "Creating VPBasicBlock for " << Name << "\n");
@@ -145,14 +148,6 @@ bool PlainCFGBuilder::isExternalDef(Value *Val) {
     // Instruction definition is in outermost loop PH.
     return false;
 
-  // Check whether Instruction definition is in a loop exit.
-  SmallVector<BasicBlock *> ExitBlocks;
-  TheLoop->getExitBlocks(ExitBlocks);
-  if (is_contained(ExitBlocks, InstParent)) {
-    // Instruction definition is in outermost loop exit.
-    return false;
-  }
-
   // Check whether Instruction definition is in loop body.
   return !TheLoop->contains(Inst);
 }
@@ -201,11 +196,6 @@ void PlainCFGBuilder::createVPInstructionsForVPBB(VPBasicBlock *VPBB,
            "Instruction shouldn't have been visited.");
 
     if (auto *Br = dyn_cast<BranchInst>(Inst)) {
-      if (TheLoop->getLoopLatch() == BB ||
-          any_of(successors(BB),
-                 [this](BasicBlock *Succ) { return !TheLoop->contains(Succ); }))
-        continue;
-
       // Conditional branch instruction are represented using BranchOnCond
       // recipes.
       if (Br->isConditional()) {
@@ -295,7 +285,6 @@ std::unique_ptr<VPlan> PlainCFGBuilder::buildPlainCFG(
   for (BasicBlock *BB : RPO) {
     // Create or retrieve the VPBasicBlock for this BB.
     VPBasicBlock *VPBB = getOrCreateVPBB(BB);
-    Loop *LoopForBB = LI->getLoopFor(BB);
     // Set VPBB predecessors in the same order as they are in the incoming BB.
     setVPBBPredsFromBB(VPBB, BB);
 
@@ -326,24 +315,12 @@ std::unique_ptr<VPlan> PlainCFGBuilder::buildPlainCFG(
     BasicBlock *IRSucc1 = BI->getSuccessor(1);
     VPBasicBlock *Successor0 = getOrCreateVPBB(IRSucc0);
     VPBasicBlock *Successor1 = getOrCreateVPBB(IRSucc1);
-
-    // Don't connect any blocks outside the current loop except the latches for
-    // inner loops.
-    // TODO: Also connect exit blocks during initial VPlan construction.
-    if (LoopForBB == TheLoop || BB != LoopForBB->getLoopLatch()) {
-      if (!LoopForBB->contains(IRSucc0)) {
-        VPBB->setOneSuccessor(Successor1);
-        continue;
-      }
-      if (!LoopForBB->contains(IRSucc1)) {
-        VPBB->setOneSuccessor(Successor0);
-        continue;
-      }
-    }
-
     VPBB->setTwoSuccessors(Successor0, Successor1);
   }
 
+  for (auto *EB : Plan->getExitBlocks())
+    setVPBBPredsFromBB(EB, EB->getIRBasicBlock());
+
   // 2. The whole CFG has been built at this point so all the input Values must
   // have a VPlan counterpart. Fix VPlan header phi by adding their
   // corresponding VPlan operands.
@@ -446,19 +423,21 @@ static void createLoopRegion(VPlan &Plan, VPBlockBase *HeaderVPB) {
   VPBlockBase *Succ = LatchVPBB->getSingleSuccessor();
   assert(LatchVPBB->getNumSuccessors() <= 1 &&
          "Latch has more than one successor");
-  if (Succ)
-    VPBlockUtils::disconnectBlocks(LatchVPBB, Succ);
+  LatchVPBB->removeSuccessor(Succ);
 
   auto *R = Plan.createVPRegionBlock(HeaderVPB, LatchVPBB, "",
                                      false /*isReplicator*/);
   // All VPBB's reachable shallowly from HeaderVPB belong to top level loop,
   // because VPlan is expected to end at top level latch disconnected above.
+  SmallPtrSet<VPBlockBase *, 2> ExitBlocks(Plan.getExitBlocks().begin(),
+                                           Plan.getExitBlocks().end());
   for (VPBlockBase *VPBB : vp_depth_first_shallow(HeaderVPB))
-    VPBB->setParent(R);
+    if (!ExitBlocks.contains(VPBB))
+      VPBB->setParent(R);
 
   VPBlockUtils::insertBlockAfter(R, PreheaderVPBB);
-  if (Succ)
-    VPBlockUtils::connectBlocks(R, Succ);
+  R->setOneSuccessor(Succ);
+  Succ->replacePredecessor(LatchVPBB, R);
 }
 
 void VPlanTransforms::prepareForVectorization(VPlan &Plan, Type *InductionTy,
@@ -476,8 +455,29 @@ void VPlanTransforms::prepareForVectorization(VPlan &Plan, Type *InductionTy,
   VPBlockUtils::insertBlockAfter(VecPreheader, Plan.getEntry());
 
   VPBasicBlock *MiddleVPBB = Plan.createVPBasicBlock("middle.block");
-  VPBlockUtils::connectBlocks(LatchVPB, MiddleVPBB);
-  LatchVPB->swapSuccessors();
+  VPBlockBase *LatchExitVPB = LatchVPB->getNumSuccessors() == 2
+                                  ? LatchVPB->getSuccessors()[0]
+                                  : nullptr;
+  if (LatchExitVPB) {
+    LatchVPB->getSuccessors()[0] = MiddleVPBB;
+    MiddleVPBB->setPredecessors({LatchVPB});
+    MiddleVPBB->setSuccessors({LatchExitVPB});
+    LatchExitVPB->replacePredecessor(LatchVPB, MiddleVPBB);
+  } else {
+    VPBlockUtils::connectBlocks(LatchVPB, MiddleVPBB);
+    LatchVPB->swapSuccessors();
+  }
+
+  // Disconnect all edges between exit blocks other than from the latch.
+  // TODO: Uncountable exit blocks should be handled here.
+  for (VPBlockBase *EB : to_vector(Plan.getExitBlocks())) {
+    for (VPBlockBase *Pred : to_vector(EB->getPredecessors())) {
+      if (Pred == MiddleVPBB)
+        continue;
+      cast<VPBasicBlock>(Pred)->getTerminator()->eraseFromParent();
+      VPBlockUtils::disconnectBlocks(Pred, EB);
+    }
+  }
 
   // Create SCEV and VPValue for the trip count.
   // We use the symbolic max backedge-taken-count, which works also when
@@ -503,8 +503,9 @@ void VPlanTransforms::prepareForVectorization(VPlan &Plan, Type *InductionTy,
   //    Thus if tail is to be folded, we know we don't need to run the
   //    remainder and we can set the condition to true.
   // 3) Otherwise, construct a runtime check.
-
   if (!RequiresScalarEpilogueCheck) {
+    if (LatchExitVPB)
+      VPBlockUtils::disconnectBlocks(MiddleVPBB, LatchExitVPB);
     VPBlockUtils::connectBlocks(MiddleVPBB, ScalarPH);
     // The exit blocks are unreachable, remove their recipes to make sure no
     // users remain that may pessimize transforms.
@@ -516,9 +517,6 @@ void VPlanTransforms::prepareForVectorization(VPlan &Plan, Type *InductionTy,
   }
 
   // The connection order corresponds to the operands of the conditional branch.
-  BasicBlock *IRExitBlock = TheLoop->getUniqueLatchExitBlock();
-  auto *VPExitBlock = Plan.getExitBlock(IRExitBlock);
-  VPBlockUtils::connectBlocks(MiddleVPBB, VPExitBlock);
   VPBlockUtils::connectBlocks(MiddleVPBB, ScalarPH);
 
   auto *ScalarLatchTerm = TheLoop->getLoopLatch()->getTerminator();
diff --git a/llvm/test/Transforms/LoopVectorize/vplan-printing-outer-loop.ll b/llvm/test/Transforms/LoopVectorize/vplan-printing-outer-loop.ll
index 91a5ea6b7fe36..fe845ae74cbee 100644
--- a/llvm/test/Transforms/LoopVectorize/vplan-printing-outer-loop.ll
+++ b/llvm/test/Transforms/LoopVectorize/vplan-printing-outer-loop.ll
@@ -31,7 +31,11 @@ define void @foo(i64 %n) {
 ; CHECK-NEXT: outer.latch:
 ; CHECK-NEXT:   EMIT ir<%outer.iv.next> = add ir<%outer.iv>, ir<1>
 ; CHECK-NEXT:   EMIT ir<%outer.ec> = icmp ir<%outer.iv.next>, ir<8>
-; CHECK-NEXT: Successor(s): outer.header
+; CHECK-NEXT:   EMIT branch-on-cond ir<%outer.ec>
+; CHECK-NEXT: Successor(s): ir-bb<exit>, outer.header
+; CHECK-EMPTY:
+; CHECK-NEXT: ir-bb<exit>:
+; CHECK-NEXT: No successors
 ; CHECK-NEXT: }
 entry:
   br label %outer.header

Move early-exit handling up front to original VPlan construction, before introducing early exits. This builds on llvm#137709, which adds exiting edges to the original VPlan, instead of adding exit blocks later. This retains the exit conditions early, and means we can handle early exits before forming regions, without the reliance on VPRecipeBuilder. Once we retain all exits initially, handling early exits before region construction ensures the regions are valid; otherwise we would leave edges exiting the region from elsewhere than the latch. Removing the reliance on VPRecipeBuilder removes the dependence on mapping IR BBs to VPBBs and unblocks predication as VPlan transform: llvm#128420. Depends on llvm#137709.

Update initial VPlan construction to include exit conditions and edges. For now, all early exits are disconnected before forming the regions, but a follow-up will update uncountable exit handling to also happen here. This is required to enable VPlan predication and remove the dependence any IR BBs (llvm#128420). This includes updates in a few places to use replaceSuccessor/replacePredecessor to preserve the order of predecessors and successors, to reduce the need of fixing up phi operand orderings. This unfortunately required making them public, not sure if there's a

Move early-exit handling up front to original VPlan construction, before introducing early exits. This builds on llvm#137709, which adds exiting edges to the original VPlan, instead of adding exit blocks later. This retains the exit conditions early, and means we can handle early exits before forming regions, without the reliance on VPRecipeBuilder. Once we retain all exits initially, handling early exits before region construction ensures the regions are valid; otherwise we would leave edges exiting the region from elsewhere than the latch. Removing the reliance on VPRecipeBuilder removes the dependence on mapping IR BBs to VPBBs and unblocks predication as VPlan transform: llvm#128420. Depends on llvm#137709.

ayalz

Raises some thoughts... (trying to address the incomplete "not sure if there's a" ending of the commit message)

ayalz · 2025-05-04T05:07:44Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

-  // Check whether Instruction definition is in a loop exit.
-  SmallVector<BasicBlock *> ExitBlocks;
-  TheLoop->getExitBlocks(ExitBlocks);
-  if (is_contained(ExitBlocks, InstParent)) {
-    // Instruction definition is in outermost loop exit.
-    return false;
-  }
-


Is this simply redundant, independently, as such instructions which reside in ExitBlocks are not contained in TheLoop?

Should the above definition of external defs be updated?

Yep, removed as NFC in 282af2d

ayalz · 2025-05-04T05:33:05Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

  for (VPBlockBase *VPBB : vp_depth_first_shallow(HeaderVPB))
-    VPBB->setParent(R);
+    if (!ExitBlocks.contains(VPBB))


We can now reach exit blocks, contrary to above comment, via early exits?

Yep, comment updated, thanks!

Hmm2, createLoopRegion() is called after prepareForVectorization(), which according to the changes below should have removed all early-exit edges, so is this check (if VPBB is an exit block) needed?

Not reachable in the latest version, removed thanks!

ayalz · 2025-05-04T14:29:57Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp


  addCanonicalIVRecipes(Plan, cast<VPBasicBlock>(HeaderVPB),
                        cast<VPBasicBlock>(LatchVPB), InductionTy, IVDL);

+  // Disconnect all edges between exit blocks other than from the latch.


Suggested change

// Disconnect all edges between exit blocks other than from the latch.

// Disconnect all edges to exit blocks other than from the middle block.

?

Done thanks!

ayalz · 2025-05-06T11:23:25Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

+  if (!TheLoop->contains(BB))
+    return Plan->getExitBlock(BB);
+


How about populating BB2VPBB with Plan's exit blocks on PlainCFGBuilder's construction, to fold this check with the one above?

Done thanks!

ayalz · 2025-05-06T11:32:18Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

  assert(LatchVPBB->getNumSuccessors() <= 1 &&
         "Latch has more than one successor");


Suggested change

assert(LatchVPBB->getNumSuccessors() <= 1 &&

"Latch has more than one successor");

assert(Succ && "Latch expected to be left with a single successor");

?

Done thanks!

ayalz · 2025-05-06T13:14:24Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

  for (VPBlockBase *VPBB : vp_depth_first_shallow(HeaderVPB))
-    VPBB->setParent(R);
+    if (!ExitBlocks.contains(VPBB))
+      VPBB->setParent(R);

  VPBlockUtils::insertBlockAfter(R, PreheaderVPBB);


The PlaceHolder introduced earlier may now be used instead as follows

Suggested change

VPBlockUtils::insertBlockAfter(R, PreheaderVPBB);

Done thanks

ayalz · 2025-05-06T14:02:38Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

+  VPBlockBase *LatchExitVPB = LatchVPB->getNumSuccessors() == 2
+                                  ? LatchVPB->getSuccessors()[0]
+                                  : nullptr;
+  if (LatchExitVPB) {
+    LatchVPB->getSuccessors()[0] = MiddleVPBB;
+    MiddleVPBB->setPredecessors({LatchVPB});
+    MiddleVPBB->setSuccessors({LatchExitVPB});
+    LatchExitVPB->replacePredecessor(LatchVPB, MiddleVPBB);


Suggested change

VPBlockBase *LatchExitVPB = LatchVPB->getNumSuccessors() == 2

? LatchVPB->getSuccessors()[0]

: nullptr;

if (LatchExitVPB) {

LatchVPB->getSuccessors()[0] = MiddleVPBB;

MiddleVPBB->setPredecessors({LatchVPB});

MiddleVPBB->setSuccessors({LatchExitVPB});

LatchExitVPB->replacePredecessor(LatchVPB, MiddleVPBB);

// Canonical LatchVPB has header block as last successor. If it has another

// successor, the latter is an exit block - insert middle block on its edge.

// Otherwise, add middle block as another successor retaining header as last.

if (LatchVPB->getNumSuccessors() == 2) {

VPBlockBase *LatchExitVPB = LatchVPB->getSuccessors()[0];

VPBlockUtils::insertOnEdge(LatchVPB, LatchExitVPB, MiddleVPBB);

?

Done thanks!

ayalz · 2025-05-06T14:30:15Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp


  addCanonicalIVRecipes(Plan, cast<VPBasicBlock>(HeaderVPB),
                        cast<VPBasicBlock>(LatchVPB), InductionTy, IVDL);

+  // Disconnect all edges between exit blocks other than from the latch.
+  // TODO: Uncountable exit blocks should be handled here.


They are "handled", being listed in getExitBlocks; but should be handled differently?

Updated, here for now we just simply drop the edges, i.e. leaving the plan in an incorrect/incomplete state, relying on the later transform to adjust the exit condition and introduce an extra middle block, looking up the early exit condition using original IR references.

ayalz · 2025-05-06T14:35:22Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

  if (!RequiresScalarEpilogueCheck) {
+    if (LatchExitVPB)


Suggested change

if (LatchExitVPB)

if (auto *LatchExitVPB = MiddleVPBB->getSingleSuccessor())

?

done thanks!

ayalz · 2025-05-06T14:38:11Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -117,6 +117,7 @@ class VPBlockBase {
    Predecessors.erase(Pos);
  }

+public:


Hopefully this low-level API can remain private, possibly with an addition to the high-level public API, see below.

Done thanks!

Remove checking if the instruction is in the preheader or exit blocks. Those checks are redundant and handled by checking if the instruction is outside the loop below. Split off as suggested from #137709.

Flip the region construction order to innermost first from outermost first. This ensures we only set the final parent region for VPBBs once. Split off from #137709.

Move early-exit handling up front to original VPlan construction, before introducing early exits. This builds on llvm#137709, which adds exiting edges to the original VPlan, instead of adding exit blocks later. This retains the exit conditions early, and means we can handle early exits before forming regions, without the reliance on VPRecipeBuilder. Once we retain all exits initially, handling early exits before region construction ensures the regions are valid; otherwise we would leave edges exiting the region from elsewhere than the latch. Removing the reliance on VPRecipeBuilder removes the dependence on mapping IR BBs to VPBBs and unblocks predication as VPlan transform: llvm#128420. Depends on llvm#137709.

ayalz

Commit message needs updating (and completion..)

TODO: Suspect mergeBlocksIntoPredecessors() might lose predecessor positions as it doesn't check if the empty block's successors have a single predecessor (though that may be the case currently; otherwise should probably not be considered atm). Supplying removeOnEdge(Block) which disconnects Block from its single Pred and single Succ, reconnecting them together while retaining pred and succ positions may help, but an empty block being merged into its predecessor may have multiple successors - rather than being on an edge.

ayalz · 2025-05-06T20:41:18Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

-          any_of(successors(BB),
-                 [this](BasicBlock *Succ) { return !TheLoop->contains(Succ); }))
-        continue;
-
      // Conditional branch instruction are represented using BranchOnCond
      // recipes.
      if (Br->isConditional()) {


nit: seems a bit clearer to early continue?

Suggested change

if (Br->isConditional()) {

if (!Br->isConditional())

continue;

Sorry, ignore, we continue also after handling a conditional branch.

ayalz · 2025-05-06T20:43:15Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

-  // All VPBB's reachable shallowly from HeaderVPB belong to top level loop,
-  // because VPlan is expected to end at top level latch disconnected above.
+  // All VPBB's reachable shallowly from HeaderVPB belong to the current region,
+  // except the exit blocks reachable via non-latch exiting blocks,


Suggested change

// except the exit blocks reachable via non-latch exiting blocks,

// except the exit blocks reachable via non-latch exiting blocks.

Fixed thanks!

ayalz · 2025-05-06T21:13:49Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

  VPBlockUtils::insertBlockAfter(R, PreheaderVPBB);
-  if (Succ)
-    VPBlockUtils::connectBlocks(R, Succ);
+  VPBlockUtils::insertOnEdge(PlaceHolder, Succ, R);
+
+  // Remove placeholder block.
+  VPBlockUtils::disconnectBlocks(R, PlaceHolder);
+  VPBlockUtils::disconnectBlocks(PlaceHolder, R);


Inserting R first after Preheader (clearer to instead explicitly insert R on the edge from Preheader to PlaceHolder?) and then inserting R again on the edge between PlaceHolder and Succ seems a bit confusing, thereby creating bidirectional edges between R and PlaceHolder which are then removed. It's like inserting R both before PlaceHolder and after it, but both these insertions require R to be disconnected. In essence we want to replace PlaceHolder with R.
Another alternative is to have an empty region as a placeholder rather than an empty basic block, and then move/copy R's entry and exit blocks into it:

auto *PlaceHolder = Plan.createVPRegionBlock("", false /*isReplicator*/);

followed by

PlaceHolder.setEntry(R.getEntry()); PlaceHolder.setExit(R.getExit());

essentially turning R to be the temporary block and PlaceHolder to be R, constructed outside in rather than inside out?

Perhaps suffice to be a bit clearer by slightly reordering (would Exit be a better name for Succ?):

// Have R replace PlaceHolder as successor of Preheader. BlockUtils::insertOnEdge(PreheaderBlock, PlaceHolder, R); BlockUtils::disconnectBlocks(R, PlaceHolder); // Have R replace PlaceHolder as predecessor of Exit. BlockUtils::insertOnEdge(PlaceHolder, Exit, R); BlockUtils::disconnectBlocks(PlaceHolder, R);

Actually there's no need to construct a placeholder region, we can simply create an empty main region first and then set entry/exiting after adjusting the CFG. Updated, thanks!

ayalz · 2025-05-06T21:25:05Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

+  // TODO: VPlans with early exits should be explicitly converted to a form only
+  // exiting via the latch here, including adjusting the exit condition, instead
+  // of simplify disconnecting the edges and adjusting the VPlan later.


Suggested change

// TODO: VPlans with early exits should be explicitly converted to a form only

// exiting via the latch here, including adjusting the exit condition, instead

// of simplify disconnecting the edges and adjusting the VPlan later.

// TODO: VPlans with early exits should be explicitly converted to a form

// exiting only via the latch here, including adjusting the exit condition,

// instead of simply disconnecting the edges and adjusting the VPlan later.

Done thanks!

…rnalDef (NFC). Remove checking if the instruction is in the preheader or exit blocks. Those checks are redundant and handled by checking if the instruction is outside the loop below. Split off as suggested from llvm/llvm-project#137709.

Flip the region construction order to innermost first from outermost first. This ensures we only set the final parent region for VPBBs once. Split off from llvm/llvm-project#137709.

fhahn · 2025-05-07T08:56:05Z

Commit message needs updating (and completion..)

Thanks, updated now.

TODO: Suspect mergeBlocksIntoPredecessors() might lose predecessor positions as it doesn't check if the empty block's successors have a single predecessor (though that may be the case currently; otherwise should probably not be considered atm). Supplying removeOnEdge(Block) which disconnects Block from its single Pred and single Succ, reconnecting them together while retaining pred and succ positions may help, but an empty block being merged into its predecessor may have multiple successors - rather than being on an edge.

Now there's no need to remove/merge, so I left the TODO out of the commit message for now. It will mangle the predecessor order of the successors, at the moment not used in places where successors could have phi-like recipes I think

david-arm · 2025-05-07T12:37:57Z

Hi @fhahn, looks like there might be some genuine unit test failures on the Windows build?

david-arm · 2025-05-07T12:50:59Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

-  VPBlockUtils::connectBlocks(LatchVPB, MiddleVPBB);
-  LatchVPB->swapSuccessors();
+  // Canonical LatchVPB has header block as last successor. If it has another
+  // successor, the latter is an exit block - insert middle block on its edge.


Sorry, I'm a bit confused by the word "latter" here. Does "latter" refer to "another successor"? Perhaps it's easier just to say that if the latchvpb is not canonical the early exit block(s) come first, with the (canonical?) exit to the middle block being last? If I've understood the layout correctly.

LatchVPB is canonical - as a result of calling canonicalHeaderAndLatch() above. Being a canonical latch means LatchVPB has header block as its last successor, and this property is maintained. If LatchVPB has another successor, in addition to header, this other successor (appears first and) is an exit block. In this case middle block is inserted on the edge from LatchVPB to its first exit block successor. Should "the latter" be replaced with "this other successor"?

Updated to say

If it has another successor, this successor is an exit block

david-arm · 2025-05-07T12:53:54Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

+      if (Pred == MiddleVPBB)
+        continue;
+      cast<VPBasicBlock>(Pred)->getTerminator()->eraseFromParent();
+      VPBlockUtils::disconnectBlocks(Pred, EB);


This looks a bit odd at first as it seems to be undoing what you did above with VPBlockUtils::insertOnEdge(LatchVPB, LatchExitVPB, MiddleVPBB);. I presume that's because the vector.early.exit VPBB sits between the latch block and the original IR early exit block?

The edge from MiddleVPBB to LatchExitVPB is retained here explicitly via the early-continue exclusion. The edges removed here are early-exits from non-latch Pred block to early.exit block. Block vector.early.exit is introduced by handleUncountableEarlyExit() which currently takes place later.

Above with insertOnEdge we only handle the edge exiting from the latch. The exit block via the latch will now be connected to the middle block.

The loop here disconnects all early exits and they will be handled later: either by requiring at least one scalar iteration, nothing more needs to be done, or introducing the early exit control flow to go to the early exit via the additional middle block. For the latter case, the VPlan is now incomplete/incorrect.

To avoid this, we should directly handle the uncountable early exits here, which is done in #138393. This way, we do not need to rely on IR references in handleUncountableEarlyExit and the VPlan remains complete/correct throughout.

ayalz · 2025-05-07T09:12:48Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

+  // order of blocks. Set region entry and exiting after both HeaderVPB and
+  // LatchVPBB have been disconnected from their predecessors/successors.
+  auto *R = Plan.createVPRegionBlock("", false /*isReplicator*/);
+


nit:

Suggested change

ayalz · 2025-05-07T13:13:47Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

-  VPBlockUtils::connectBlocks(LatchVPB, MiddleVPBB);
-  LatchVPB->swapSuccessors();
+  // Canonical LatchVPB has header block as last successor. If it has another
+  // successor, the latter is an exit block - insert middle block on its edge.


LatchVPB is canonical - as a result of calling canonicalHeaderAndLatch() above. Being a canonical latch means LatchVPB has header block as its last successor, and this property is maintained. If LatchVPB has another successor, in addition to header, this other successor (appears first and) is an exit block. In this case middle block is inserted on the edge from LatchVPB to its first exit block successor. Should "the latter" be replaced with "this other successor"?

fhahn · 2025-05-07T13:37:02Z

Hi @fhahn, looks like there might be some genuine unit test failures on the Windows build?

A few C++ unit tests still needed updating, should be done now

Remove checking if the instruction is in the preheader or exit blocks. Those checks are redundant and handled by checking if the instruction is outside the loop below. Split off as suggested from llvm#137709.

Flip the region construction order to innermost first from outermost first. This ensures we only set the final parent region for VPBBs once. Split off from llvm#137709.

ayalz · 2025-05-07T15:40:35Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

+  // The canonical LatchVPB has the header block as last successor. If it has
+  // another successor, this successor is an exit block - insert middle block on
+  // its edge. Otherwise, add middle block as another successor retaining header
+  // as last.


Hmm, prepareForVectorization() is called after buildPlainCFG() (for both native and non-native paths), and with the above changes, buildPlainCFG() connects the latch to both header and exit blocks, i.e., should LatchVPB always have two successors now?

We can vectorize loops where the latch isn’t exiting, by requiring at least one iteration in the scalar loop. In that case, the original latch will
Have a single successor

Ah, right, thanks. Perhaps worth a comment.

ayalz

This is fine w/ me, adding some last final comments, thanks!
@david-arm - is this ok w/ you?

ayalz · 2025-05-07T16:27:33Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

@@ -534,9 +547,6 @@ void VPlanTransforms::prepareForVectorization(VPlan &Plan, Type *InductionTy,
  }

  // The connection order corresponds to the operands of the conditional branch.


"// with middle block already connected to exit block."?

added thanks

ayalz · 2025-05-07T16:30:58Z

llvm/unittests/Transforms/Vectorize/VPlanHCFGTest.cpp

@@ -302,6 +304,7 @@ compound=true
      "  EMIT store ir\<%res\>, ir\<%arr.idx\>\l" +
      "  EMIT ir\<%iv.next\> = add ir\<%iv\>, ir\<1\>\l" +
      "  EMIT ir\<%exitcond\> = icmp ir\<%iv.next\>, ir\<%N\>\l" +
+      "  EMIT vp\<%3\> = not ir\<%exitcond\>\l" +


Should vp%3 be used, also above.

No, at this point we already added the canonical branch to control the loop; the old conditions will be cleaned up by DCE

ayalz · 2025-05-07T18:46:45Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

+  // The canonical LatchVPB has the header block as last successor. If it has
+  // another successor, this successor is an exit block - insert middle block on
+  // its edge. Otherwise, add middle block as another successor retaining header
+  // as last.


Ah, right, thanks. Perhaps worth a comment.

ayalz · 2025-05-07T19:42:11Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

  for (VPBlockBase *VPBB : vp_depth_first_shallow(HeaderVPB))
-    VPBB->setParent(R);
+    if (!ExitBlocks.contains(VPBB))


Hmm2, createLoopRegion() is called after prepareForVectorization(), which according to the changes below should have removed all early-exit edges, so is this check (if VPBB is an exit block) needed?

ayalz · 2025-05-07T19:52:54Z

llvm/unittests/Transforms/Vectorize/VPlanHCFGTest.cpp

@@ -229,6 +230,7 @@ TEST_F(VPlanHCFGTest, testVPInstructionToVPRecipesInner) {
  EXPECT_NE(nullptr, dyn_cast<VPInstruction>(&*Iter++));
  EXPECT_NE(nullptr, dyn_cast<VPInstruction>(&*Iter++));
  EXPECT_NE(nullptr, dyn_cast<VPInstruction>(&*Iter++));
+  EXPECT_NE(nullptr, dyn_cast<VPInstruction>(&*Iter++));


The previous case didn't check all instructions (that Iter reached VecBB->end()), so avoid checking the new last instruction, as done here?

left as is for now, just carrying the curent behavior forward as this checks that we reach VecBB->end(). Might be something to be done separately? Or checking if we have the expected canonical instructions, which may make the test more brittle.

ayalz · 2025-05-07T21:02:14Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

  SmallPtrSet<VPBlockBase *, 2> ExitBlocks(Plan.getExitBlocks().begin(),
                                           Plan.getExitBlocks().end());


Suggested change

SmallPtrSet<VPBlockBase *, 2> ExitBlocks(Plan.getExitBlocks().begin(),

Plan.getExitBlocks().end());

?

removed thanks

ayalz · 2025-05-07T21:06:00Z

llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp

+  SmallPtrSet<VPBlockBase *, 2> ExitBlocks(Plan.getExitBlocks().begin(),
+                                           Plan.getExitBlocks().end());


Suggested change

SmallPtrSet<VPBlockBase *, 2> ExitBlocks(Plan.getExitBlocks().begin(),

Plan.getExitBlocks().end());

?

…(NFC). (#137709) Update initial VPlan construction to include exit conditions and edges. The loop region is now first constructed without entry/exiting. Those are set after inserting the region in the CFG, to preserve the original predecessor/successor order of blocks. For now, all early exits are disconnected before forming the regions, but a follow-up will update uncountable exit handling to also happen here. This is required to enable VPlan predication and remove the dependence any IR BBs (llvm/llvm-project#128420). PR: llvm/llvm-project#137709

Move early-exit handling up front to original VPlan construction, before introducing early exits. This builds on llvm#137709, which adds exiting edges to the original VPlan, instead of adding exit blocks later. This retains the exit conditions early, and means we can handle early exits before forming regions, without the reliance on VPRecipeBuilder. Once we retain all exits initially, handling early exits before region construction ensures the regions are valid; otherwise we would leave edges exiting the region from elsewhere than the latch. Removing the reliance on VPRecipeBuilder removes the dependence on mapping IR BBs to VPBBs and unblocks predication as VPlan transform: llvm#128420. Depends on llvm#137709.

Move early-exit handling up front to original VPlan construction, before introducing early exits. This builds on #137709, which adds exiting edges to the original VPlan, instead of adding exit blocks later. This retains the exit conditions early, and means we can handle early exits before forming regions, without the reliance on VPRecipeBuilder. Once we retain all exits initially, handling early exits before region construction ensures the regions are valid; otherwise we would leave edges exiting the region from elsewhere than the latch. Removing the reliance on VPRecipeBuilder removes the dependence on mapping IR BBs to VPBBs and unblocks predication as VPlan transform: #128420. Depends on #137709 (included in PR). PR: #138393

…138393) Move early-exit handling up front to original VPlan construction, before introducing early exits. This builds on llvm/llvm-project#137709, which adds exiting edges to the original VPlan, instead of adding exit blocks later. This retains the exit conditions early, and means we can handle early exits before forming regions, without the reliance on VPRecipeBuilder. Once we retain all exits initially, handling early exits before region construction ensures the regions are valid; otherwise we would leave edges exiting the region from elsewhere than the latch. Removing the reliance on VPRecipeBuilder removes the dependence on mapping IR BBs to VPBBs and unblocks predication as VPlan transform: llvm/llvm-project#128420. Depends on llvm/llvm-project#137709 (included in PR). PR: llvm/llvm-project#138393

fhahn requested review from rengolin, ayalz and aniragil April 28, 2025 21:09

llvmbot added vectorizers llvm:transforms labels Apr 28, 2025

fhahn force-pushed the vplan-retain-exits branch from c368b5d to e347f11 Compare May 3, 2025 12:08

fhahn mentioned this pull request May 3, 2025

[VPlan] Handle early exit before forming regions. (NFC) #138393

Merged

fhahn force-pushed the vplan-retain-exits branch from e347f11 to de0fc2d Compare May 5, 2025 17:10

ayalz reviewed May 6, 2025

View reviewed changes

fhahn added 3 commits May 6, 2025 19:15

Merge remote-tracking branch 'origin/main' into vplan-retain-exits

19bec82

!fixup address first set of comments, thanks

55b4d5e

!fixup address remaining comments, thanks

ca45f88

fhahn added 2 commits May 6, 2025 20:51

Merge remote-tracking branch 'origin/main' into vplan-retain-exits

ddfdeef

!fixup add back assertion in replacePredecessors.

1284bc6

ayalz reviewed May 6, 2025

View reviewed changes

fhahn added 3 commits May 7, 2025 09:09

Merge remote-tracking branch 'origin/main' into vplan-retain-exits

79317a5

!fixup address latest comments, thanks!

3cc4b31

!fixup only construct one region

bbb902e

!fixup update llvm/unittests/Transforms/Vectorize/VPlanHCFGTest.cpp

c1902ed

david-arm reviewed May 7, 2025

View reviewed changes

ayalz reviewed May 7, 2025

View reviewed changes

!fixup address comments, thanks!

0cadcf9

ayalz reviewed May 7, 2025

View reviewed changes

ayalz approved these changes May 7, 2025

View reviewed changes

fhahn added 2 commits May 7, 2025 21:13

Merge remote-tracking branch 'origin/main' into vplan-retain-exits

5fa6b7d

!fixup address latest comments, thanks

5d6729d

ayalz reviewed May 7, 2025

View reviewed changes

fhahn added 2 commits May 7, 2025 22:11

!fixup remove ExitBlocks.

1211faf

Merge branch 'main' into vplan-retain-exits

50d7ee1

fhahn merged commit 339dc95 into llvm:main May 8, 2025
8 of 10 checks passed

fhahn deleted the vplan-retain-exits branch May 8, 2025 17:16

	// Disconnect all edges between exit blocks other than from the latch.
	// Disconnect all edges to exit blocks other than from the middle block.

		assert(LatchVPBB->getNumSuccessors() <= 1 &&
		"Latch has more than one successor");

	assert(LatchVPBB->getNumSuccessors() <= 1 &&
	"Latch has more than one successor");
	assert(Succ && "Latch expected to be left with a single successor");

	if (LatchExitVPB)
	if (auto *LatchExitVPB = MiddleVPBB->getSingleSuccessor())

	if (Br->isConditional()) {
	if (!Br->isConditional())
	continue;

	// except the exit blocks reachable via non-latch exiting blocks,
	// except the exit blocks reachable via non-latch exiting blocks.

		@@ -534,9 +547,6 @@ void VPlanTransforms::prepareForVectorization(VPlan &Plan, Type *InductionTy,
		}

		// The connection order corresponds to the operands of the conditional branch.

		SmallPtrSet<VPBlockBase *, 2> ExitBlocks(Plan.getExitBlocks().begin(),
		Plan.getExitBlocks().end());

[VPlan] Retain exit conditions and edges in initial VPlan (NFC). #137709

[VPlan] Retain exit conditions and edges in initial VPlan (NFC). #137709

Uh oh!

Conversation

fhahn commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ayalz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayalz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayalz May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fhahn commented Apr 28, 2025 •

edited

Loading

llvmbot commented Apr 28, 2025 •

edited

Loading

ayalz left a comment •

edited

Loading

ayalz May 7, 2025 •

edited

Loading