[VPlan] Thread plan to VPBuilder (NFC) #125364
Conversation
|
@llvm/pr-subscribers-vectorizers @llvm/pr-subscribers-llvm-transforms Author: Ramkumar Ramachandra (artagnon) ChangesConstruct VPBuilder with a VPlan object, and change VPRecipeBuilder to own the VPBuilder, in preparation to use the VPlan object in VPBuilder to implement constant-folding. The VPlan reference in VPBuilder is unused for now. Full diff: https://github.com/llvm/llvm-project/pull/125364.diff 7 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h b/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
index bc44ec11edb7b0..76e7bf2f62650a 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
@@ -25,7 +25,6 @@
#define LLVM_TRANSFORMS_VECTORIZE_LOOPVECTORIZATIONPLANNER_H
#include "VPlan.h"
-#include "llvm/ADT/SmallSet.h"
#include "llvm/Support/InstructionCost.h"
namespace llvm {
@@ -45,6 +44,7 @@ class VPRecipeBuilder;
class VPBuilder {
VPBasicBlock *BB = nullptr;
VPBasicBlock::iterator InsertPt = VPBasicBlock::iterator();
+ [[maybe_unused]] VPlan &Plan;
/// Insert \p VPI in BB at InsertPt if BB is set.
template <typename T> T *tryInsertInstruction(T *R) {
@@ -66,10 +66,15 @@ class VPBuilder {
}
public:
- VPBuilder() = default;
- VPBuilder(VPBasicBlock *InsertBB) { setInsertPoint(InsertBB); }
- VPBuilder(VPRecipeBase *InsertPt) { setInsertPoint(InsertPt); }
- VPBuilder(VPBasicBlock *TheBB, VPBasicBlock::iterator IP) {
+ VPBuilder(VPlan &Plan) : Plan(Plan) {}
+ VPBuilder(VPlan &Plan, VPBasicBlock *InsertBB) : Plan(Plan) {
+ setInsertPoint(InsertBB);
+ }
+ VPBuilder(VPlan &Plan, VPRecipeBase *InsertPt) : Plan(Plan) {
+ setInsertPoint(InsertPt);
+ }
+ VPBuilder(VPlan &Plan, VPBasicBlock *TheBB, VPBasicBlock::iterator IP)
+ : Plan(Plan) {
setInsertPoint(TheBB, IP);
}
@@ -83,13 +88,6 @@ class VPBuilder {
VPBasicBlock *getInsertBlock() const { return BB; }
VPBasicBlock::iterator getInsertPoint() const { return InsertPt; }
- /// Create a VPBuilder to insert after \p R.
- static VPBuilder getToInsertAfter(VPRecipeBase *R) {
- VPBuilder B;
- B.setInsertPoint(R->getParent(), std::next(R->getIterator()));
- return B;
- }
-
/// InsertPoint - A saved insertion point.
class VPInsertPoint {
VPBasicBlock *Block = nullptr;
@@ -390,9 +388,6 @@ class LoopVectorizationPlanner {
/// Profitable vector factors.
SmallVector<VectorizationFactor, 8> ProfitableVFs;
- /// A builder used to construct the current plan.
- VPBuilder Builder;
-
/// Computes the cost of \p Plan for vectorization factor \p VF.
///
/// The current implementation requires access to the
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 493ce848171211..858eddae56943b 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -8947,7 +8947,7 @@ static void addCanonicalIVRecipes(VPlan &Plan, Type *IdxTy, bool HasNUW,
VPBasicBlock *Header = TopRegion->getEntryBasicBlock();
Header->insert(CanonicalIVPHI, Header->begin());
- VPBuilder Builder(TopRegion->getExitingBasicBlock());
+ VPBuilder Builder(Plan, TopRegion->getExitingBasicBlock());
// Add a VPInstruction to increment the scalar canonical IV by VF * UF.
auto *CanonicalIVIncrement = Builder.createOverflowingOp(
Instruction::Add, {CanonicalIVPHI, &Plan.getVFxUF()}, {HasNUW, false}, DL,
@@ -9007,9 +9007,9 @@ static void addScalarResumePhis(VPRecipeBuilder &Builder, VPlan &Plan,
auto *MiddleVPBB = cast<VPBasicBlock>(ScalarPH->getSinglePredecessor());
VPRegionBlock *VectorRegion = Plan.getVectorLoopRegion();
VPBuilder VectorPHBuilder(
- cast<VPBasicBlock>(VectorRegion->getSinglePredecessor()));
- VPBuilder MiddleBuilder(MiddleVPBB, MiddleVPBB->getFirstNonPhi());
- VPBuilder ScalarPHBuilder(ScalarPH);
+ Plan, cast<VPBasicBlock>(VectorRegion->getSinglePredecessor()));
+ VPBuilder MiddleBuilder(Plan, MiddleVPBB, MiddleVPBB->getFirstNonPhi());
+ VPBuilder ScalarPHBuilder(Plan, ScalarPH);
VPValue *OneVPV = Plan.getOrAddLiveIn(
ConstantInt::get(Plan.getCanonicalIV()->getScalarType(), 1));
for (VPRecipeBase &ScalarPhiR : *Plan.getScalarHeader()) {
@@ -9101,7 +9101,7 @@ addUsersInExitBlocks(VPlan &Plan,
return;
auto *MiddleVPBB = Plan.getMiddleBlock();
- VPBuilder B(MiddleVPBB, MiddleVPBB->getFirstNonPhi());
+ VPBuilder B(Plan, MiddleVPBB, MiddleVPBB->getFirstNonPhi());
// Introduce extract for exiting values and update the VPIRInstructions
// modeling the corresponding LCSSA phis.
@@ -9123,8 +9123,8 @@ static void addExitUsersForFirstOrderRecurrences(
VPRegionBlock *VectorRegion = Plan.getVectorLoopRegion();
auto *ScalarPHVPBB = Plan.getScalarPreheader();
auto *MiddleVPBB = Plan.getMiddleBlock();
- VPBuilder ScalarPHBuilder(ScalarPHVPBB);
- VPBuilder MiddleBuilder(MiddleVPBB, MiddleVPBB->getFirstNonPhi());
+ VPBuilder ScalarPHBuilder(Plan, ScalarPHVPBB);
+ VPBuilder MiddleBuilder(Plan, MiddleVPBB, MiddleVPBB->getFirstNonPhi());
VPValue *TwoVPV = Plan.getOrAddLiveIn(
ConstantInt::get(Plan.getCanonicalIV()->getScalarType(), 2));
@@ -9261,8 +9261,7 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
bool HasNUW = !IVUpdateMayOverflow || Style == TailFoldingStyle::None;
addCanonicalIVRecipes(*Plan, Legal->getWidestInductionType(), HasNUW, DL);
- VPRecipeBuilder RecipeBuilder(*Plan, OrigLoop, TLI, &TTI, Legal, CM, PSE,
- Builder);
+ VPRecipeBuilder RecipeBuilder(*Plan, OrigLoop, TLI, &TTI, Legal, CM, PSE);
// ---------------------------------------------------------------------------
// Pre-construction: record ingredients whose recipes we'll need to further
@@ -9318,7 +9317,7 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
// ingredients and fill a new VPBasicBlock.
if (VPBB != HeaderVPBB)
VPBB->setName(BB->getName());
- Builder.setInsertPoint(VPBB);
+ RecipeBuilder.setInsertPoint(VPBB);
if (VPBB == HeaderVPBB)
RecipeBuilder.createHeaderMask();
@@ -9482,7 +9481,7 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
// Sink users of fixed-order recurrence past the recipe defining the previous
// value and introduce FirstOrderRecurrenceSplice VPInstructions.
if (!VPlanTransforms::runPass(VPlanTransforms::adjustFixedOrderRecurrences,
- *Plan, Builder))
+ *Plan, RecipeBuilder.getIRBuilder()))
return nullptr;
if (useActiveLaneMask(Style)) {
@@ -9532,8 +9531,7 @@ VPlanPtr LoopVectorizationPlanner::buildVPlan(VFRange &Range) {
// Collect mapping of IR header phis to header phi recipes, to be used in
// addScalarResumePhis.
- VPRecipeBuilder RecipeBuilder(*Plan, OrigLoop, TLI, &TTI, Legal, CM, PSE,
- Builder);
+ VPRecipeBuilder RecipeBuilder(*Plan, OrigLoop, TLI, &TTI, Legal, CM, PSE);
for (auto &R : Plan->getVectorLoopRegion()->getEntryBasicBlock()->phis()) {
if (isa<VPCanonicalIVPHIRecipe>(&R))
continue;
@@ -9698,6 +9696,7 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
}
}
VPBasicBlock *LatchVPBB = VectorLoopRegion->getExitingBasicBlock();
+ VPBuilder Builder(*Plan);
Builder.setInsertPoint(&*LatchVPBB->begin());
VPBasicBlock::iterator IP = MiddleVPBB->getFirstNonPhi();
for (VPRecipeBase &R :
@@ -10205,7 +10204,7 @@ static void preparePlanForMainVectorLoop(VPlan &MainPlan, VPlan &EpiPlan) {
m_Specific(VectorTC), m_SpecificInt(0)));
}))
return;
- VPBuilder ScalarPHBuilder(MainScalarPH, MainScalarPH->begin());
+ VPBuilder ScalarPHBuilder(MainPlan, MainScalarPH, MainScalarPH->begin());
ScalarPHBuilder.createNaryOp(
VPInstruction::ResumePhi,
{VectorTC, MainPlan.getCanonicalIV()->getStartValue()}, {},
diff --git a/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h b/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
index 87c97d1edd7b6a..c9c3a1abec5283 100644
--- a/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
+++ b/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
@@ -12,7 +12,6 @@
#include "LoopVectorizationPlanner.h"
#include "VPlan.h"
#include "llvm/ADT/DenseMap.h"
-#include "llvm/ADT/PointerUnion.h"
#include "llvm/Analysis/ScalarEvolutionExpressions.h"
#include "llvm/IR/IRBuilder.h"
@@ -65,7 +64,7 @@ class VPRecipeBuilder {
PredicatedScalarEvolution &PSE;
- VPBuilder &Builder;
+ VPBuilder Builder;
/// When we if-convert we need to create edge masks. We have to cache values
/// so that we don't end up with exponential recursion/IR. Note that
@@ -155,9 +154,13 @@ class VPRecipeBuilder {
const TargetTransformInfo *TTI,
LoopVectorizationLegality *Legal,
LoopVectorizationCostModel &CM,
- PredicatedScalarEvolution &PSE, VPBuilder &Builder)
+ PredicatedScalarEvolution &PSE)
: Plan(Plan), OrigLoop(OrigLoop), TLI(TLI), TTI(TTI), Legal(Legal),
- CM(CM), PSE(PSE), Builder(Builder) {}
+ CM(CM), PSE(PSE), Builder(Plan) {}
+
+ void setInsertPoint(VPBasicBlock *VPBB) { Builder.setInsertPoint(VPBB); }
+
+ VPBuilder &getIRBuilder() { return Builder; }
std::optional<unsigned> getScalingForReduction(const Instruction *ExitInst) {
auto It = ScaledReductionMap.find(ExitInst);
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.cpp b/llvm/lib/Transforms/Vectorize/VPlan.cpp
index 4a1512abe4e48c..5f7a69fd35a088 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlan.cpp
@@ -915,7 +915,7 @@ VPlanPtr VPlan::createInitialVPlan(Type *InductionTy,
// of the corresponding compare because they may have ended up with
// different line numbers and we want to avoid awkward line stepping while
// debugging. Eg. if the compare has got a line number inside the loop.
- VPBuilder Builder(MiddleVPBB);
+ VPBuilder Builder(*Plan, MiddleVPBB);
VPValue *Cmp =
TailFolded
? Plan->getOrAddLiveIn(ConstantInt::getTrue(
diff --git a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
index 5a2e5d7cfee48d..a0505e0d1bb8d0 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
@@ -72,7 +72,7 @@ class PlainCFGBuilder {
public:
PlainCFGBuilder(Loop *Lp, LoopInfo *LI, VPlan &P)
- : TheLoop(Lp), LI(LI), Plan(P) {}
+ : TheLoop(Lp), LI(LI), Plan(P), VPIRBuilder(Plan) {}
/// Build plain CFG for TheLoop and connects it to Plan's entry.
void buildPlainCFG();
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
index a1a2cf211abf88..bf95b62474150c 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -591,7 +591,7 @@ static void legalizeAndOptimizeInductions(VPlan &Plan) {
using namespace llvm::VPlanPatternMatch;
VPBasicBlock *HeaderVPBB = Plan.getVectorLoopRegion()->getEntryBasicBlock();
bool HasOnlyVectorVFs = !Plan.hasVF(ElementCount::getFixed(1));
- VPBuilder Builder(HeaderVPBB, HeaderVPBB->getFirstNonPhi());
+ VPBuilder Builder(Plan, HeaderVPBB, HeaderVPBB->getFirstNonPhi());
for (VPRecipeBase &Phi : HeaderVPBB->phis()) {
auto *PhiR = dyn_cast<VPWidenInductionRecipe>(&Phi);
if (!PhiR)
@@ -744,7 +744,7 @@ void VPlanTransforms::optimizeInductionExitUsers(
"predecessor must be the middle block");
VPTypeAnalysis TypeInfo(Plan.getCanonicalIV()->getScalarType());
- VPBuilder B(Plan.getMiddleBlock()->getTerminator());
+ VPBuilder B(Plan, Plan.getMiddleBlock()->getTerminator());
for (VPRecipeBase &R : *ExitVPBB) {
auto *ExitIRI = cast<VPIRInstruction>(&R);
if (!isa<PHINode>(ExitIRI->getInstruction()))
@@ -1505,7 +1505,7 @@ static VPActiveLaneMaskPHIRecipe *addVPLaneMaskPhiAndUpdateExitBranch(
// we have to take unrolling into account. Each part needs to start at
// Part * VF
auto *VecPreheader = Plan.getVectorPreheader();
- VPBuilder Builder(VecPreheader);
+ VPBuilder Builder(Plan, VecPreheader);
// Create the ActiveLaneMask instruction using the correct start values.
VPValue *TC = Plan.getTripCount();
@@ -1624,7 +1624,8 @@ void VPlanTransforms::addActiveLaneMask(
LaneMask = addVPLaneMaskPhiAndUpdateExitBranch(
Plan, DataAndControlFlowWithoutRuntimeCheck);
} else {
- VPBuilder B = VPBuilder::getToInsertAfter(WideCanonicalIV);
+ VPBuilder B(Plan, WideCanonicalIV->getParent(),
+ std::next(WideCanonicalIV->getIterator()));
LaneMask = B.createNaryOp(VPInstruction::ActiveLaneMask,
{WideCanonicalIV, Plan.getTripCount()}, nullptr,
"active.lane.mask");
@@ -1828,7 +1829,7 @@ bool VPlanTransforms::tryAddExplicitVectorLength(
// Create the ExplicitVectorLengthPhi recipe in the main loop.
auto *EVLPhi = new VPEVLBasedIVPHIRecipe(StartV, DebugLoc());
EVLPhi->insertAfter(CanonicalIVPHI);
- VPBuilder Builder(Header, Header->getFirstNonPhi());
+ VPBuilder Builder(Plan, Header, Header->getFirstNonPhi());
// Compute original TC - IV as the AVL (application vector length).
VPValue *AVL = Builder.createNaryOp(
Instruction::Sub, {Plan.getTripCount(), EVLPhi}, DebugLoc(), "avl");
@@ -1909,7 +1910,7 @@ void VPlanTransforms::dropPoisonGeneratingRecipes(
// where the operands are disjoint or poison otherwise.
if (match(RecWithFlags, m_BinaryOr(m_VPValue(A), m_VPValue(B))) &&
RecWithFlags->isDisjoint()) {
- VPBuilder Builder(RecWithFlags);
+ VPBuilder Builder(Plan, RecWithFlags);
VPInstruction *New = Builder.createOverflowingOp(
Instruction::Add, {A, B}, {false, false},
RecWithFlags->getDebugLoc());
@@ -2023,7 +2024,7 @@ void VPlanTransforms::createInterleaveGroups(
/*IsSigned=*/true);
VPValue *OffsetVPV = Plan.getOrAddLiveIn(
ConstantInt::get(IRInsertPos->getParent()->getContext(), -Offset));
- VPBuilder B(InsertPos);
+ VPBuilder B(Plan, InsertPos);
Addr = InBounds ? B.createInBoundsPtrAdd(InsertPos->getAddr(), OffsetVPV)
: B.createPtrAdd(InsertPos->getAddr(), OffsetVPV);
}
@@ -2069,7 +2070,7 @@ void VPlanTransforms::handleUncountableEarlyExit(
BasicBlock *UncountableExitingBlock, VPRecipeBuilder &RecipeBuilder) {
VPRegionBlock *LoopRegion = Plan.getVectorLoopRegion();
auto *LatchVPBB = cast<VPBasicBlock>(LoopRegion->getExiting());
- VPBuilder Builder(LatchVPBB->getTerminator());
+ VPBuilder Builder(Plan, LatchVPBB->getTerminator());
auto *MiddleVPBB = Plan.getMiddleBlock();
VPValue *IsEarlyExitTaken = nullptr;
@@ -2110,8 +2111,8 @@ void VPlanTransforms::handleUncountableEarlyExit(
VPBlockUtils::connectBlocks(VectorEarlyExitVPBB, VPEarlyExitBlock);
// Update the exit phis in the early exit block.
- VPBuilder MiddleBuilder(NewMiddle);
- VPBuilder EarlyExitB(VectorEarlyExitVPBB);
+ VPBuilder MiddleBuilder(Plan, NewMiddle);
+ VPBuilder EarlyExitB(Plan, VectorEarlyExitVPBB);
for (VPRecipeBase &R : *VPEarlyExitBlock) {
auto *ExitIRI = cast<VPIRInstruction>(&R);
auto *ExitPhi = dyn_cast<PHINode>(&ExitIRI->getInstruction());
diff --git a/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp b/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp
index 89e372d6b46cfd..53e086dfe1cbd4 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp
@@ -156,7 +156,7 @@ void UnrollState::unrollWidenInductionByUF(
FMFs = ID.getInductionBinOp()->getFastMathFlags();
VPValue *VectorStep = &Plan.getVF();
- VPBuilder Builder(PH);
+ VPBuilder Builder(Plan, PH);
if (TypeInfo.inferScalarType(VectorStep) != IVTy) {
Instruction::CastOps CastOp =
IVTy->isFloatingPointTy() ? Instruction::UIToFP : Instruction::Trunc;
|
Construct VPBuilder with a VPlan object, and change VPRecipeBuilder to own the VPBuilder, in preparation to use the VPlan object in VPBuilder to implement constant-folding. The VPlan reference in VPBuilder is unused for now.
artagnon
force-pushed
the
vplan-thread-vpbuilder
branch
from
f139689 to
734120d
Compare
|
Gentle ping. |
1 similar comment
|
Gentle ping. |
artagnon
changed the title
artagnon
changed the title
There was a problem hiding this comment.
It seems like there is more to this PR than just passing a VPlan object reference to the VPBuilder constructor, since it's also changing VPRecipeBuilder to instantiate a VPBuilder object every time a VPRecipeBuilder object is constructed. Unless I'm missing something I can't see any reason why they are tied together in the same patch?
Can you explain more what the patch is trying to achieve with the refactoring so we can see the bigger picture? I am actually more concerned about the VPRecipeBuilder changes because it's not immediately obvious to me what the impact of dropping the reference is. Presumably there was a reason why it was a reference, perhaps to keep the insertion point in sync with other uses of the builder outside the class? @fhahn any thoughts?
|
|
||
| void setInsertPoint(VPBasicBlock *VPBB) { Builder.setInsertPoint(VPBB); } | ||
|
|
||
| VPBuilder &getIRBuilder() { return Builder; } |
There was a problem hiding this comment.
This is a bit confusing for me personally, since there is a class called IRBuilder that really does refer to LLVM IR. I'd prefer a different name here, something like getVPBuilder
This is due to the way they're inherently tied up. If
VPlan-level constant folding: #125365. The constant-folder shows real improvements in some cases, with no regressions.
As far as I can tell, I've preserved the insertion point in this patch, and there should be no impact of dropping the reference, provided we're careful about preserving the insertion point. |
Makes sense, will check why my previous attempt was crashing again.
Hm, actually I'm not too happy with What would it mean to write a constant-folder in For these reasons, I'm inclined to not touch |
The builder seems to be used in two ways: to create a new recipe, and to create a new recipe and insert it: template <typename T> T *tryInsertInstruction(T *R) {
if (BB)
BB->insert(R, InsertPt);
return R;
}... but I agree that we need to fix this issue instead of doing the NFC of passing the Plan to VPBuilder. |
Construct VPBuilder with a VPlan object, and change VPRecipeBuilder to own the VPBuilder, in preparation to use the VPlan object in VPBuilder to implement constant-folding. The VPlan reference in VPBuilder is unused for now.