[VPlan] Add VPIRBasicBlock, use to model pre-preheader. #93398

fhahn · 2024-05-26T04:50:01Z

This patch adds a new special type of VPBasicBlock that wraps an existing IR basic block. Recipes of the block get added before the terminator of the wrapped IR basic block. Making it a subclass of VPBasicBlock avoids duplicating various APIs to manage recipes in a block, as well as makes sure the traversals filtering VPBasicBlocks automatically apply as well.

Initially VPIRBasicBlock are only used for the pre-preheader (wrapping the original preheader of the scalar loop).

As follow-up, this will be used to move more parts of the skeleton inside VPlan, starting with the branch and condition in the middle block.

Separated out of #92651

This patch adds a new special type of VPBasicBlock that wraps an existing IR basic block. Recipes of the block get added before the terminator of the wrapped IR basic block. Making it a subclass of VPBasicBlock avoids duplicating various APIs to manage recipes in a block, as well as makes sure the traversals filtering VPBasicBlocks automatically apply as well. Initially VPIRWrappedBlocks are only used for the pre-preheader (wrapping the original preheader of the scalar loop). As follow-up, this will be used to move more parts of the skeleton inside VPlan, startingt with the branch and condition in the middle block. Note: This reqiores updating all VPlan-printing tests, which I will do once we converge on a final version.

llvmbot · 2024-05-26T04:50:36Z

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

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

This patch adds a new special type of VPBasicBlock that wraps an existing IR basic block. Recipes of the block get added before the terminator of the wrapped IR basic block. Making it a subclass of VPBasicBlock avoids duplicating various APIs to manage recipes in a block, as well as makes sure the traversals filtering VPBasicBlocks automatically apply as well.

Initially VPIRWrappedBlocks are only used for the pre-preheader (wrapping the original preheader of the scalar loop).

As follow-up, this will be used to move more parts of the skeleton inside VPlan, startingt with the branch and condition in the middle block.

Note: This requires updating all VPlan-printing tests, which I will do once we converge on a final version.

Separated out of #92651

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

5 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+2-2)
(modified) llvm/lib/Transforms/Vectorize/VPlan.cpp (+55-2)
(modified) llvm/lib/Transforms/Vectorize/VPlan.h (+60-5)
(modified) llvm/test/Transforms/LoopVectorize/vplan-printing-before-execute.ll (+2-2)
(modified) llvm/unittests/Transforms/Vectorize/VPlanTestBase.h (+8-6)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 48981a6bd39e3..e71a0df1d9c7c 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -8607,7 +8607,7 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
   // loop region contains a header and latch basic blocks.
   VPlanPtr Plan = VPlan::createInitialVPlan(
       createTripCountSCEV(Legal->getWidestInductionType(), PSE, OrigLoop),
-      *PSE.getSE());
+      *PSE.getSE(), OrigLoop->getLoopPreheader());
   VPBasicBlock *HeaderVPBB = new VPBasicBlock("vector.body");
   VPBasicBlock *LatchVPBB = new VPBasicBlock("vector.latch");
   VPBlockUtils::insertBlockAfter(LatchVPBB, HeaderVPBB);
@@ -8855,7 +8855,7 @@ VPlanPtr LoopVectorizationPlanner::buildVPlan(VFRange &Range) {
   // Create new empty VPlan
   auto Plan = VPlan::createInitialVPlan(
       createTripCountSCEV(Legal->getWidestInductionType(), PSE, OrigLoop),
-      *PSE.getSE());
+      *PSE.getSE(), OrigLoop->getLoopPreheader());
 
   // Build hierarchical CFG
   VPlanHCFGBuilder HCFGBuilder(OrigLoop, LI, *Plan);
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.cpp b/llvm/lib/Transforms/Vectorize/VPlan.cpp
index d71d7580e6ba6..8998e392a433e 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlan.cpp
@@ -442,6 +442,58 @@ VPBasicBlock::createEmptyBasicBlock(VPTransformState::CFGState &CFG) {
   return NewBB;
 }
 
+void VPIRWrapperBlock::execute(VPTransformState *State) {
+  for (VPBlockBase *PredVPBlock : getHierarchicalPredecessors()) {
+    VPBasicBlock *PredVPBB = PredVPBlock->getExitingBasicBlock();
+    auto &PredVPSuccessors = PredVPBB->getHierarchicalSuccessors();
+    BasicBlock *PredBB = State->CFG.VPBB2IRBB[PredVPBB];
+
+    assert(PredBB && "Predecessor basic-block not found building successor.");
+    auto *PredBBTerminator = PredBB->getTerminator();
+    LLVM_DEBUG(dbgs() << "LV: draw edge from" << PredBB->getName() << '\n');
+
+    auto *TermBr = dyn_cast<BranchInst>(PredBBTerminator);
+    if (TermBr) {
+      // Set each forward successor here when it is created, excluding
+      // backedges. A backward successor is set when the branch is created.
+      unsigned idx = PredVPSuccessors.front() == this ? 0 : 1;
+      assert(!TermBr->getSuccessor(idx) &&
+             "Trying to reset an existing successor block.");
+      TermBr->setSuccessor(idx, WrappedBlock);
+    }
+  }
+
+  assert(getHierarchicalSuccessors().size() == 0 &&
+         "VPIRWrapperBlock cannot have successors");
+  State->CFG.VPBB2IRBB[this] = getWrappedBlock();
+  State->CFG.PrevVPBB = this;
+
+  auto *Term = cast<BranchInst>(getWrappedBlock()->getTerminator());
+  State->Builder.SetInsertPoint(Term);
+
+  for (VPRecipeBase &Recipe : *this)
+    Recipe.execute(*State);
+
+  LLVM_DEBUG(dbgs() << "LV: filled BB:" << *getWrappedBlock());
+}
+
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+
+void VPIRWrapperBlock::print(raw_ostream &O, const Twine &Indent,
+                             VPSlotTracker &SlotTracker) const {
+  O << Indent << "ir-bb<" << getName() << ">:\n";
+
+  auto RecipeIndent = Indent + "  ";
+  for (const VPRecipeBase &Recipe : *this) {
+    Recipe.print(O, RecipeIndent, SlotTracker);
+    O << '\n';
+  }
+  assert(getSuccessors().empty() &&
+         "Wrapper blocks should not have successors");
+  printSuccessors(O, Indent);
+}
+#endif
+
 void VPBasicBlock::execute(VPTransformState *State) {
   bool Replica = State->Instance && !State->Instance->isFirstIteration();
   VPBasicBlock *PrevVPBB = State->CFG.PrevVPBB;
@@ -769,8 +821,9 @@ VPlan::~VPlan() {
     delete BackedgeTakenCount;
 }
 
-VPlanPtr VPlan::createInitialVPlan(const SCEV *TripCount, ScalarEvolution &SE) {
-  VPBasicBlock *Preheader = new VPBasicBlock("ph");
+VPlanPtr VPlan::createInitialVPlan(const SCEV *TripCount, ScalarEvolution &SE,
+                                   BasicBlock *PH) {
+  VPIRWrapperBlock *Preheader = new VPIRWrapperBlock(PH);
   VPBasicBlock *VecPreheader = new VPBasicBlock("vector.ph");
   auto Plan = std::make_unique<VPlan>(Preheader, VecPreheader);
   Plan->TripCount =
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 3aee17921086d..20a12b571d0c0 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -473,7 +473,11 @@ class VPBlockBase {
   /// that are actually instantiated. Values of this enumeration are kept in the
   /// SubclassID field of the VPBlockBase objects. They are used for concrete
   /// type identification.
-  using VPBlockTy = enum { VPBasicBlockSC, VPRegionBlockSC };
+  using VPBlockTy = enum {
+    VPBasicBlockSC,
+    VPRegionBlockSC,
+    VPIRWrapperBlockSC
+  };
 
   using VPBlocksTy = SmallVectorImpl<VPBlockBase *>;
 
@@ -2834,6 +2838,10 @@ class VPBasicBlock : public VPBlockBase {
   /// The VPRecipes held in the order of output instructions to generate.
   RecipeListTy Recipes;
 
+protected:
+  VPBasicBlock(const unsigned char BlockSC, const Twine &Name = "")
+      : VPBlockBase(BlockSC, Name.str()) {}
+
 public:
   VPBasicBlock(const Twine &Name = "", VPRecipeBase *Recipe = nullptr)
       : VPBlockBase(VPBasicBlockSC, Name.str()) {
@@ -2882,7 +2890,8 @@ class VPBasicBlock : public VPBlockBase {
 
   /// Method to support type inquiry through isa, cast, and dyn_cast.
   static inline bool classof(const VPBlockBase *V) {
-    return V->getVPBlockID() == VPBlockBase::VPBasicBlockSC;
+    return V->getVPBlockID() == VPBlockBase::VPBasicBlockSC ||
+           V->getVPBlockID() == VPBlockBase::VPIRWrapperBlockSC;
   }
 
   void insert(VPRecipeBase *Recipe, iterator InsertPt) {
@@ -2951,6 +2960,50 @@ class VPBasicBlock : public VPBlockBase {
   BasicBlock *createEmptyBasicBlock(VPTransformState::CFGState &CFG);
 };
 
+/// A special type of VPBasicBlock that wraps an existing IR basic block.
+/// Recipes of the block get added before the terminator of the wrapped IR basic
+/// block.
+class VPIRWrapperBlock : public VPBasicBlock {
+  BasicBlock *WrappedBlock;
+
+public:
+  VPIRWrapperBlock(BasicBlock *WrappedBlock)
+      : VPBasicBlock(VPIRWrapperBlockSC, WrappedBlock->getName()),
+        WrappedBlock(WrappedBlock) {}
+
+  ~VPIRWrapperBlock() override {}
+
+  static inline bool classof(const VPBlockBase *V) {
+    return V->getVPBlockID() == VPBlockBase::VPIRWrapperBlockSC;
+  }
+
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+  /// Print this VPBsicBlock to \p O, prefixing all lines with \p Indent. \p
+  /// SlotTracker is used to print unnamed VPValue's using consequtive numbers.
+  ///
+  /// Note that the numbering is applied to the whole VPlan, so printing
+  /// individual blocks is consistent with the whole VPlan printing.
+  void print(raw_ostream &O, const Twine &Indent,
+             VPSlotTracker &SlotTracker) const override;
+  using VPBlockBase::print; // Get the print(raw_stream &O) version.
+#endif
+  /// The method which generates the output IR instructions that correspond to
+  /// this VPBasicBlock, thereby "executing" the VPlan.
+  void execute(VPTransformState *State) override;
+
+  VPIRWrapperBlock *clone() override {
+    auto *NewBlock = new VPIRWrapperBlock(WrappedBlock);
+    for (VPRecipeBase &R : *this)
+      NewBlock->appendRecipe(R.clone());
+    return NewBlock;
+  }
+
+  void dropAllReferences(VPValue *NewValue) override {}
+  void resetBlock(BasicBlock *N) { WrappedBlock = N; }
+
+  BasicBlock *getWrappedBlock() { return WrappedBlock; }
+};
+
 /// VPRegionBlock represents a collection of VPBasicBlocks and VPRegionBlocks
 /// which form a Single-Entry-Single-Exiting subgraph of the output IR CFG.
 /// A VPRegionBlock may indicate that its contents are to be replicated several
@@ -3139,12 +3192,12 @@ class VPlan {
   ~VPlan();
 
   /// Create initial VPlan skeleton, having an "entry" VPBasicBlock (wrapping
-  /// original scalar pre-header) which contains SCEV expansions that need to
-  /// happen before the CFG is modified; a VPBasicBlock for the vector
+  /// original scalar pre-header \p PH) which contains SCEV expansions that need
+  /// to happen before the CFG is modified; a VPBasicBlock for the vector
   /// pre-header, followed by a region for the vector loop, followed by the
   /// middle VPBasicBlock.
   static VPlanPtr createInitialVPlan(const SCEV *TripCount,
-                                     ScalarEvolution &PSE);
+                                     ScalarEvolution &PSE, BasicBlock *PH);
 
   /// Prepare the plan for execution, setting up the required live-in values.
   void prepareToExecute(Value *TripCount, Value *VectorTripCount,
@@ -3321,6 +3374,8 @@ class VPlanPrinter {
   /// its successor blocks.
   void dumpBasicBlock(const VPBasicBlock *BasicBlock);
 
+  void dumpIRWrapperBlock(const VPIRWrapperBlock *WrapperBlock);
+
   /// Print a given \p Region of the Plan.
   void dumpRegion(const VPRegionBlock *Region);
 
diff --git a/llvm/test/Transforms/LoopVectorize/vplan-printing-before-execute.ll b/llvm/test/Transforms/LoopVectorize/vplan-printing-before-execute.ll
index ca9dfdc6f6d29..2bb3c898c7cda 100644
--- a/llvm/test/Transforms/LoopVectorize/vplan-printing-before-execute.ll
+++ b/llvm/test/Transforms/LoopVectorize/vplan-printing-before-execute.ll
@@ -13,7 +13,7 @@ define void @test_tc_less_than_16(ptr %A, i64 %N) {
 ; CHECK-NEXT: Live-in vp<[[VTC:%.+]]> = vector-trip-count
 ; CHECK-NEXT: vp<[[TC:%.+]]> = original trip-count
 ; CHECK-EMPTY:
-; CHECK-NEXT: ph:
+; CHECK-NEXT: ir-bb<entry>:
 ; CHECK-NEXT:   EMIT vp<[[TC]]> = EXPAND SCEV (zext i4 (trunc i64 %N to i4) to i64)
 ; CHECK-NEXT: No successors
 ; CHECK-EMPTY:
@@ -45,7 +45,7 @@ define void @test_tc_less_than_16(ptr %A, i64 %N) {
 ; CHECK-NEXT: Live-in vp<[[VFxUF:%.+]]> = VF * UF
 ; CHECK-NEXT: vp<[[TC:%.+]]> = original trip-count
 ; CHECK-EMPTY:
-; CHECK-NEXT: ph:
+; CHECK-NEXT: ir-bb<entry>:
 ; CHECK-NEXT:   EMIT vp<[[TC]]> = EXPAND SCEV (zext i4 (trunc i64 %N to i4) to i64)
 ; CHECK-NEXT: No successors
 ; CHECK-EMPTY:
diff --git a/llvm/unittests/Transforms/Vectorize/VPlanTestBase.h b/llvm/unittests/Transforms/Vectorize/VPlanTestBase.h
index 6cd43f6803130..c658724278fe0 100644
--- a/llvm/unittests/Transforms/Vectorize/VPlanTestBase.h
+++ b/llvm/unittests/Transforms/Vectorize/VPlanTestBase.h
@@ -67,9 +67,10 @@ class VPlanTestBase : public testing::Test {
     assert(!verifyFunction(F) && "input function must be valid");
     doAnalysis(F);
 
-    auto Plan = VPlan::createInitialVPlan(
-        SE->getBackedgeTakenCount(LI->getLoopFor(LoopHeader)), *SE);
-    VPlanHCFGBuilder HCFGBuilder(LI->getLoopFor(LoopHeader), LI.get(), *Plan);
+    Loop *L = LI->getLoopFor(LoopHeader);
+    auto Plan = VPlan::createInitialVPlan(SE->getBackedgeTakenCount(L), *SE,
+                                          L->getLoopPreheader());
+    VPlanHCFGBuilder HCFGBuilder(L, LI.get(), *Plan);
     HCFGBuilder.buildHierarchicalCFG();
     return Plan;
   }
@@ -80,9 +81,10 @@ class VPlanTestBase : public testing::Test {
     assert(!verifyFunction(F) && "input function must be valid");
     doAnalysis(F);
 
-    auto Plan = VPlan::createInitialVPlan(
-        SE->getBackedgeTakenCount(LI->getLoopFor(LoopHeader)), *SE);
-    VPlanHCFGBuilder HCFGBuilder(LI->getLoopFor(LoopHeader), LI.get(), *Plan);
+    Loop *L = LI->getLoopFor(LoopHeader);
+    auto Plan = VPlan::createInitialVPlan(SE->getBackedgeTakenCount(L), *SE,
+                                          L->getLoopPreheader());
+    VPlanHCFGBuilder HCFGBuilder(L, LI.get(), *Plan);
     HCFGBuilder.buildPlainCFG();
     return Plan;
   }

ayalz

Looks right to me, thanks!

A subclass looks better than adding a bit to VPBasicBlock, right?

Perhaps VPIRBasicBlock would be a good name, being both.

According to the roadmap, there are two such basic blocks: scalar.ph and exit, plus say a VPIRRegionBlock representing the entire original scalar loop: https://llvm.org/devmtg/2023-10/slides/techtalks/Hahn-VPlan-StatusUpdateAndRoadmap.pdf#page=37

Adding some comments inline.

ayalz · 2024-05-27T14:22:51Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

+      unsigned idx = PredVPSuccessors.front() == this ? 0 : 1;
+      assert(!TermBr->getSuccessor(idx) &&
+             "Trying to reset an existing successor block.");
+      TermBr->setSuccessor(idx, WrappedBlock);


Update DTU?

Code is gone for now, as at the moment there are neither predecessors nor successors.

ayalz · 2024-05-27T14:24:00Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

@@ -442,6 +442,58 @@ VPBasicBlock::createEmptyBasicBlock(VPTransformState::CFGState &CFG) {
  return NewBB;
 }

+void VPIRWrapperBlock::execute(VPTransformState *State) {
+  for (VPBlockBase *PredVPBlock : getHierarchicalPredecessors()) {


Pre-preheader block has no predecessors, can assert, next to asserting it has no successors (currently).

Yes, in the initial version patching up the CFG isn't needed. Removed and added assert.

ayalz · 2024-05-27T14:26:55Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

+    }
+  }
+
+  assert(getHierarchicalSuccessors().size() == 0 &&


Suggested change

assert(getHierarchicalSuccessors().size() == 0 &&

assert(getHierarchicalSuccessors().empty() &&

done, thanks!

ayalz · 2024-05-27T14:28:46Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

+  auto *Term = cast<BranchInst>(getWrappedBlock()->getTerminator());
+  State->Builder.SetInsertPoint(Term);
+
+  for (VPRecipeBase &Recipe : *this)


nit: can iterate over Recipes instead of *this, as done in VPBasicBlock::execute().

Done, thanks!

ayalz · 2024-05-27T14:31:07Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

+
+void VPIRWrapperBlock::print(raw_ostream &O, const Twine &Indent,
+                             VPSlotTracker &SlotTracker) const {
+  O << Indent << "ir-bb<" << getName() << ">:\n";


Set the name to include ir-bb as prefix, and avoid replicating VPBasicBlock::print() by overriding it?

Dropped, thanks!

ayalz · 2024-05-27T15:05:31Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  void dropAllReferences(VPValue *NewValue) override {}
+  void resetBlock(BasicBlock *N) { WrappedBlock = N; }
+
+  BasicBlock *getWrappedBlock() { return WrappedBlock; }


Added, thanks!

ayalz · 2024-05-27T15:08:09Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -3321,6 +3374,8 @@ class VPlanPrinter {
  /// its successor blocks.
  void dumpBasicBlock(const VPBasicBlock *BasicBlock);

+  void dumpIRWrapperBlock(const VPIRWrapperBlock *WrapperBlock);


Is dumpIRWrapperBlock() defined, called, needed or can dumpBasicBlock() be (or is) reused?
Add documentation.

Not needed with the latest changes, removed thanks!

ayalz · 2024-05-27T15:18:39Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  }
+
+  void dropAllReferences(VPValue *NewValue) override {}
+  void resetBlock(BasicBlock *N) { WrappedBlock = N; }


Is resetBlock() used?

removed, thanks!

ayalz · 2024-05-27T15:22:19Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

+      unsigned idx = PredVPSuccessors.front() == this ? 0 : 1;
+      assert(!TermBr->getSuccessor(idx) &&
+             "Trying to reset an existing successor block.");
+      TermBr->setSuccessor(idx, WrappedBlock);


Suggested change

TermBr->setSuccessor(idx, WrappedBlock);

TermBr->setSuccessor(idx, getWrappedBlock());

Gone for now, thanks!

ayalz · 2024-05-27T15:29:12Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

+  for (VPRecipeBase &Recipe : *this)
+    Recipe.execute(*State);
+
+  LLVM_DEBUG(dbgs() << "LV: filled BB:" << *getWrappedBlock());


This last part can be outlined into, say, VPBasicBlock::executeRecipesAtEndOfBB(BasicBlock*), to be reused by VPBasicBlock::execute().

Done, thanks!

…k-ph

fhahn

Looks right to me, thanks!

A subclass looks better than adding a bit to VPBasicBlock, right?

Yes I think so, that way we can keep things separate (although most logic is shared)

Perhaps VPIRBasicBlock would be a good name, being both.

Updated, thanks!

fhahn · 2024-05-28T04:45:47Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

@@ -442,6 +442,58 @@ VPBasicBlock::createEmptyBasicBlock(VPTransformState::CFGState &CFG) {
  return NewBB;
 }

+void VPIRWrapperBlock::execute(VPTransformState *State) {
+  for (VPBlockBase *PredVPBlock : getHierarchicalPredecessors()) {


Yes, in the initial version patching up the CFG isn't needed. Removed and added assert.

fhahn · 2024-05-28T04:46:08Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

+      unsigned idx = PredVPSuccessors.front() == this ? 0 : 1;
+      assert(!TermBr->getSuccessor(idx) &&
+             "Trying to reset an existing successor block.");
+      TermBr->setSuccessor(idx, WrappedBlock);


Code is gone for now, as at the moment there are neither predecessors nor successors.

fhahn · 2024-05-28T04:46:13Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

+      unsigned idx = PredVPSuccessors.front() == this ? 0 : 1;
+      assert(!TermBr->getSuccessor(idx) &&
+             "Trying to reset an existing successor block.");
+      TermBr->setSuccessor(idx, WrappedBlock);


Gone for now, thanks!

fhahn · 2024-05-28T04:46:51Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

+    }
+  }
+
+  assert(getHierarchicalSuccessors().size() == 0 &&


done, thanks!

fhahn · 2024-05-28T04:47:18Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

+  auto *Term = cast<BranchInst>(getWrappedBlock()->getTerminator());
+  State->Builder.SetInsertPoint(Term);
+
+  for (VPRecipeBase &Recipe : *this)


Done, thanks!

fhahn · 2024-05-28T05:07:17Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -2951,6 +2960,50 @@ class VPBasicBlock : public VPBlockBase {
  BasicBlock *createEmptyBasicBlock(VPTransformState::CFGState &CFG);
 };

+/// A special type of VPBasicBlock that wraps an existing IR basic block.
+/// Recipes of the block get added before the terminator of the wrapped IR basic
+/// block.


Yes, will update #92651 soon based on this PR. Left the order as is for now, to avoid updating IR tests. Can adjust order as follow-up

fhahn · 2024-05-28T05:07:52Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  }
+
+  void dropAllReferences(VPValue *NewValue) override {}
+  void resetBlock(BasicBlock *N) { WrappedBlock = N; }


removed, thanks!

fhahn · 2024-05-28T05:08:02Z

llvm/lib/Transforms/Vectorize/VPlan.h

+
+  VPIRWrapperBlock *clone() override {
+    auto *NewBlock = new VPIRWrapperBlock(WrappedBlock);
+    for (VPRecipeBase &R : *this)


Recipes is private, can move to protected?

fhahn · 2024-05-28T05:08:07Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  void dropAllReferences(VPValue *NewValue) override {}
+  void resetBlock(BasicBlock *N) { WrappedBlock = N; }
+
+  BasicBlock *getWrappedBlock() { return WrappedBlock; }


Added, thanks!

fhahn · 2024-05-28T05:08:37Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -3321,6 +3374,8 @@ class VPlanPrinter {
  /// its successor blocks.
  void dumpBasicBlock(const VPBasicBlock *BasicBlock);

+  void dumpIRWrapperBlock(const VPIRWrapperBlock *WrapperBlock);


Not needed with the latest changes, removed thanks!

…k-ph

github-actions · 2024-05-28T14:26:06Z

✅ With the latest revision this PR passed the C/C++ code formatter.

ayalz

Last set of minor comments; time to add the test changes? Could the current tests be retained, isolating the changes that affect them to a separate patch?

Commit message should be updated to refer to VPIRBasicBlock, and fix typo: "startingt".

ayalz · 2024-05-28T16:21:02Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

+  assert(getHierarchicalPredecessors().empty() &&
+         "VPIRBasicBlock cannot have predecessors at the moment");
+  assert(getHierarchicalSuccessors().empty() &&
+         "VPIRBasicBlock cannot have successors");


Suggested change

"VPIRBasicBlock cannot have successors");

"VPIRBasicBlock cannot have successors at the moment");

(Pre-preheader should eventually have successors)

added, thanks!

ayalz · 2024-05-28T16:25:10Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -473,7 +473,7 @@ class VPBlockBase {
  /// that are actually instantiated. Values of this enumeration are kept in the
  /// SubclassID field of the VPBlockBase objects. They are used for concrete
  /// type identification.
-  using VPBlockTy = enum { VPBasicBlockSC, VPRegionBlockSC };
+  using VPBlockTy = enum { VPBasicBlockSC, VPRegionBlockSC, VPIRBasicBlockSC };


Suggested change

using VPBlockTy = enum { VPBasicBlockSC, VPRegionBlockSC, VPIRBasicBlockSC };

using VPBlockTy = enum { VPRegionBlockSC, VPBasicBlockSC, VPIRBasicBlockSC };

nit: order more logical?

done, thanks!

ayalz · 2024-05-28T16:27:04Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -2837,6 +2837,10 @@ class VPBasicBlock : public VPBlockBase {
  /// The VPRecipes held in the order of output instructions to generate.
  RecipeListTy Recipes;


Yeah, Recipes can be made protected, now that a subclass may want to access them.

Done, thanks

ayalz · 2024-05-28T16:28:13Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -2948,12 +2953,48 @@ class VPBasicBlock : public VPBlockBase {
    return NewBlock;
  }

+protected:
+  void executeRecipes(VPTransformState *State, BasicBlock *BB);


Documentation missing.

Added, thanks!

ayalz · 2024-05-28T16:29:29Z

llvm/lib/Transforms/Vectorize/VPlan.h

+/// Recipes of the block get added before the first non-phi instruction in the
+/// wrapped block.
+class VPIRBasicBlock : public VPBasicBlock {
+  BasicBlock *WrappedBlock;


Suggested change

BasicBlock *WrappedBlock;

BasicBlock *IRBasicBlock;

or IRBB?

Renamed, thanks

ayalz · 2024-05-28T16:31:27Z

llvm/lib/Transforms/Vectorize/VPlan.h

+    return NewBlock;
+  }
+
+  BasicBlock *getWrappedBlock() const { return WrappedBlock; }


Suggested change

BasicBlock *getWrappedBlock() const { return WrappedBlock; }

BasicBlock *getIRBasicBlock() const { return IRBB; }

?

updated, thanks!

ayalz · 2024-05-28T16:31:50Z

llvm/lib/Transforms/Vectorize/VPlan.h

+
+  VPIRWrapperBlock *clone() override {
+    auto *NewBlock = new VPIRWrapperBlock(WrappedBlock);
+    for (VPRecipeBase &R : *this)


fhahn

Last set of minor comments; time to add the test changes? Could the current tests be retained, isolating the changes that affect them to a separate patch?

Updated the tests now. We could manually set the name to ph, but it's probably cleaner/explicit to include the adjusted name already in this patch

Commit message should be updated to refer to VPIRBasicBlock, and fix typo: "startingt".
Fixed, thanks!

fhahn · 2024-05-29T04:59:52Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

+  assert(getHierarchicalPredecessors().empty() &&
+         "VPIRBasicBlock cannot have predecessors at the moment");
+  assert(getHierarchicalSuccessors().empty() &&
+         "VPIRBasicBlock cannot have successors");


added, thanks!

fhahn · 2024-05-29T05:00:44Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -473,7 +473,7 @@ class VPBlockBase {
  /// that are actually instantiated. Values of this enumeration are kept in the
  /// SubclassID field of the VPBlockBase objects. They are used for concrete
  /// type identification.
-  using VPBlockTy = enum { VPBasicBlockSC, VPRegionBlockSC };
+  using VPBlockTy = enum { VPBasicBlockSC, VPRegionBlockSC, VPIRBasicBlockSC };


done, thanks!

fhahn · 2024-05-29T05:01:21Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -2837,6 +2837,10 @@ class VPBasicBlock : public VPBlockBase {
  /// The VPRecipes held in the order of output instructions to generate.
  RecipeListTy Recipes;


Done, thanks

fhahn · 2024-05-29T05:02:25Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -2948,12 +2953,48 @@ class VPBasicBlock : public VPBlockBase {
    return NewBlock;
  }

+protected:
+  void executeRecipes(VPTransformState *State, BasicBlock *BB);


Added, thanks!

fhahn · 2024-05-29T05:02:46Z

llvm/lib/Transforms/Vectorize/VPlan.h

+/// Recipes of the block get added before the first non-phi instruction in the
+/// wrapped block.
+class VPIRBasicBlock : public VPBasicBlock {
+  BasicBlock *WrappedBlock;


Renamed, thanks

fhahn · 2024-05-29T05:04:01Z

llvm/lib/Transforms/Vectorize/VPlan.h

+    return NewBlock;
+  }
+
+  BasicBlock *getWrappedBlock() const { return WrappedBlock; }


updated, thanks!

fhahn · 2024-05-29T05:04:06Z

llvm/lib/Transforms/Vectorize/VPlan.h

+
+  VPIRWrapperBlock *clone() override {
+    auto *NewBlock = new VPIRWrapperBlock(WrappedBlock);
+    for (VPRecipeBase &R : *this)


Updated, thanks!

…k-ph

fhahn · 2024-05-29T14:49:46Z

Just pushed the updates now

ayalz

This looks good to me, thanks! Adding a couple of last suggestions.

ayalz · 2024-05-29T18:16:11Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  void execute(VPTransformState *State) override;
+
+  VPIRBasicBlock *clone() override {
+    auto *NewBlock = new VPIRBasicBlock(IRBB);


This cloning produces multiple VPBB's holding the same IRBB, all destined to inject instructions into it following its last phi. Can we verify that the order in which these VPBB's will execute corresponds to related dependences - if any, perhaps by asserting that in any given VPlan all VPIRBasicBlocks hold distinct IRBB's? And/or document this concern.

I updated the verifier to check that IR blocks aren't wrapped by multiple wrapper blocks per VPlan.

ayalz · 2024-05-29T18:28:36Z

llvm/lib/Transforms/Vectorize/VPlan.h

+public:
+  VPIRBasicBlock(BasicBlock *IRBB)
+      : VPBasicBlock(VPIRBasicBlockSC,
+                     (Twine("ir-bb<") + IRBB->getName() + Twine(">")).str()),


Would the diff of this NFC patch decrease if name is (temporarily) set to "ph" here, avoiding any test change? A subsequent patch can follow, devoted to renaming only. (Or we could try to apply the desired renaming patch first.)

In any case, the Note at the end of the commit message regarding updating tests should be updated / removed.

Updated and check in verifier that it is only used to wrap the preheader for now

…k-ph

ayalz · 2024-05-30T18:46:15Z

Note that Case A of VPBasicBlock::execute() which reuses existing IR BB's is relieved by this patch from dealing with the pre-preheader, but is still needed for handling the preheader. When the preheader is connected to the pre-preheader, through bypass blocks, Case A can retire.

Follow-up to adjust the names and tests after #93398.

fhahn · 2024-05-31T00:45:45Z

Pushed the change to introduce the new prefix for printing: f38d84c

fhahn requested review from ayalz and aniragil May 26, 2024 04:50

llvmbot added vectorizers llvm:transforms labels May 26, 2024

fhahn mentioned this pull request May 26, 2024

[VPlan] Model branch cond to enter scalar epilogue in VPlan. #92651

Merged

ayalz reviewed May 27, 2024

View reviewed changes

fhahn added 2 commits May 27, 2024 18:56

Merge remote-tracking branch 'origin/main' into vplan-ir-wrapper-bloc…

6f5a775

…k-ph

!fixup address latest comments, thanks!

a7714cb

fhahn changed the title ~~[VPlan] Add VPIRWrapperBlock, use to model pre-preheader.~~ [VPlan] Add VPIRBasicBlock, use to model pre-preheader. May 28, 2024

fhahn commented May 28, 2024

View reviewed changes

fhahn added 2 commits May 28, 2024 07:20

!fixup VPIRWrapperBlock -> VPIRBasicBlock.

f469a3a

Merge remote-tracking branch 'origin/main' into vplan-ir-wrapper-bloc…

38c2da3

…k-ph

!fixup fix formatting

73729d4

ayalz reviewed May 28, 2024

View reviewed changes

!fixup update tests.

872f0b5

fhahn commented May 29, 2024

View reviewed changes

fhahn added 2 commits May 29, 2024 07:48

Merge remote-tracking branch 'origin/main' into vplan-ir-wrapper-bloc…

92e31f8

…k-ph

!fixup address comments, thanks!

c69dde0

llvmbot added the backend:RISC-V label May 29, 2024

ayalz approved these changes May 29, 2024

View reviewed changes

fhahn added 6 commits May 29, 2024 15:10

Merge remote-tracking branch 'origin/main' into vplan-ir-wrapper-bloc…

bd83113

…k-ph

Merge remote-tracking branch 'origin/main' into vplan-ir-wrapper-bloc…

5374577

…k-ph

!fixup verify the same BB is wrapped only once per VPlan.

b4381ff

Merge remote-tracking branch 'origin/main' into vplan-ir-wrapper-bloc…

6285815

…k-ph

!fixup restrict to VPlan preheader only.

9ef075a

!fixup also undo unit test change.

1134c38

fhahn merged commit 5785048 into llvm:main May 30, 2024
7 checks passed

fhahn deleted the vplan-ir-wrapper-block-ph branch May 30, 2024 20:35

fhahn added a commit that referenced this pull request May 31, 2024

[VPlan] Use ir-bb prefix for VPIRBasicBlock.

f38d84c

Follow-up to adjust the names and tests after #93398.

artagnon mentioned this pull request Aug 23, 2024

VPlan: prepare to recursively simplify recipes (NFC) #105699

Open

	assert(getHierarchicalSuccessors().size() == 0 &&
	assert(getHierarchicalSuccessors().empty() &&

	TermBr->setSuccessor(idx, WrappedBlock);
	TermBr->setSuccessor(idx, getWrappedBlock());

	"VPIRBasicBlock cannot have successors");
	"VPIRBasicBlock cannot have successors at the moment");

	using VPBlockTy = enum { VPBasicBlockSC, VPRegionBlockSC, VPIRBasicBlockSC };
	using VPBlockTy = enum { VPRegionBlockSC, VPBasicBlockSC, VPIRBasicBlockSC };

		@@ -2837,6 +2837,10 @@ class VPBasicBlock : public VPBlockBase {
		/// The VPRecipes held in the order of output instructions to generate.
		RecipeListTy Recipes;

	BasicBlock *getWrappedBlock() const { return WrappedBlock; }
	BasicBlock *getIRBasicBlock() const { return IRBB; }

[VPlan] Add VPIRBasicBlock, use to model pre-preheader. #93398

[VPlan] Add VPIRBasicBlock, use to model pre-preheader. #93398

Conversation

fhahn commented May 26, 2024 • edited Loading

llvmbot commented May 26, 2024 • edited Loading

ayalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 28, 2024 • edited Loading

ayalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhahn commented May 29, 2024

ayalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayalz commented May 30, 2024

fhahn commented May 31, 2024

fhahn commented May 26, 2024 •

edited

Loading

llvmbot commented May 26, 2024 •

edited

Loading

github-actions bot commented May 28, 2024 •

edited

Loading