[VPlan] Move recording of Inst->VPValue to VPRecipeBuilder (NFCI). #84464
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of keeping a mapping of Inst->VPValues (of their corresponding recipes) in VPlan's Value2VPValue mapping, keep it in VPRecipeBuilder instead. After recently replacing the last user of this mapping after initial construction, this mapping is only needed for recipe construction (to map IR operands to VPValue operands). By moving the mapping, VPlan's VPValue tracking can be simplified and limited only to live-ins. It also allows removing disableValue2VPValue and associated machinery & asserts.
|
@llvm/pr-subscribers-llvm-transforms Author: Florian Hahn (fhahn) ChangesInstead of keeping a mapping of Inst->VPValues (of their corresponding recipes) in VPlan's Value2VPValue mapping, keep it in VPRecipeBuilder instead. After recently replacing the last user of this mapping after initial construction, this mapping is only needed for recipe construction (to map IR operands to VPValue operands). By moving the mapping, VPlan's VPValue tracking can be simplified and limited only to live-ins. It also allows removing disableValue2VPValue and associated machinery & asserts. Patch is 21.87 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/84464.diff 8 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index edaad4d033bdf0..f52843c7e2d8c3 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -7913,6 +7913,18 @@ void LoopVectorizationPlanner::buildVPlans(ElementCount MinVF,
}
}
+iterator_range<mapped_iterator<Use *, std::function<VPValue *(Value *)>>>
+VPRecipeBuilder::mapToVPValues(User::op_range Operands, VPlan &Plan) {
+ std::function<VPValue *(Value *)> Fn = [this, &Plan](Value *Op) {
+ if (auto *I = dyn_cast<Instruction>(Op)) {
+ if (auto *R = Ingredient2Recipe.lookup(I))
+ return R->getVPSingleValue();
+ }
+ return Plan.getOrAddLiveIn(Op);
+ };
+ return map_range(Operands, Fn);
+}
+
VPValue *VPRecipeBuilder::createEdgeMask(BasicBlock *Src, BasicBlock *Dst,
VPlan &Plan) {
assert(is_contained(predecessors(Dst), Src) && "Invalid edge");
@@ -7938,7 +7950,7 @@ VPValue *VPRecipeBuilder::createEdgeMask(BasicBlock *Src, BasicBlock *Dst,
if (OrigLoop->isLoopExiting(Src))
return EdgeMaskCache[Edge] = SrcMask;
- VPValue *EdgeMask = Plan.getVPValueOrAddLiveIn(BI->getCondition());
+ VPValue *EdgeMask = getVPValue(BI->getCondition(), Plan);
assert(EdgeMask && "No Edge Mask found for condition");
if (BI->getSuccessor(0) != Dst)
@@ -7949,7 +7961,7 @@ VPValue *VPRecipeBuilder::createEdgeMask(BasicBlock *Src, BasicBlock *Dst,
// 'select i1 SrcMask, i1 EdgeMask, i1 false'.
// The select version does not introduce new UB if SrcMask is false and
// EdgeMask is poison. Using 'and' here introduces undefined behavior.
- VPValue *False = Plan.getVPValueOrAddLiveIn(
+ VPValue *False = Plan.getOrAddLiveIn(
ConstantInt::getFalse(BI->getCondition()->getType()));
EdgeMask =
Builder.createSelect(SrcMask, EdgeMask, False, BI->getDebugLoc());
@@ -8151,7 +8163,7 @@ VPWidenIntOrFpInductionRecipe *VPRecipeBuilder::tryToOptimizeInductionTruncate(
auto *Phi = cast<PHINode>(I->getOperand(0));
const InductionDescriptor &II = *Legal->getIntOrFpInductionDescriptor(Phi);
- VPValue *Start = Plan.getVPValueOrAddLiveIn(II.getStartValue());
+ VPValue *Start = Plan.getOrAddLiveIn(II.getStartValue());
return createWidenInductionRecipes(Phi, I, Start, II, Plan, *PSE.getSE(),
*OrigLoop, Range);
}
@@ -8263,7 +8275,7 @@ VPWidenCallRecipe *VPRecipeBuilder::tryToWidenCall(CallInst *CI,
if (Legal->isMaskRequired(CI))
Mask = getBlockInMask(CI->getParent());
else
- Mask = Plan->getVPValueOrAddLiveIn(ConstantInt::getTrue(
+ Mask = Plan->getOrAddLiveIn(ConstantInt::getTrue(
IntegerType::getInt1Ty(Variant->getFunctionType()->getContext())));
Ops.insert(Ops.begin() + *MaskPos, Mask);
@@ -8306,8 +8318,8 @@ VPWidenRecipe *VPRecipeBuilder::tryToWiden(Instruction *I,
if (CM.isPredicatedInst(I)) {
SmallVector<VPValue *> Ops(Operands.begin(), Operands.end());
VPValue *Mask = getBlockInMask(I->getParent());
- VPValue *One = Plan->getVPValueOrAddLiveIn(
- ConstantInt::get(I->getType(), 1u, false));
+ VPValue *One =
+ Plan->getOrAddLiveIn(ConstantInt::get(I->getType(), 1u, false));
auto *SafeRHS =
new VPInstruction(Instruction::Select, {Mask, Ops[1], One},
I->getDebugLoc());
@@ -8402,7 +8414,7 @@ VPReplicateRecipe *VPRecipeBuilder::handleReplication(Instruction *I,
BlockInMask = getBlockInMask(I->getParent());
}
- auto *Recipe = new VPReplicateRecipe(I, Plan.mapToVPValues(I->operands()),
+ auto *Recipe = new VPReplicateRecipe(I, mapToVPValues(I->operands(), Plan),
IsUniform, BlockInMask);
return Recipe;
}
@@ -8417,10 +8429,6 @@ VPRecipeBase *VPRecipeBuilder::tryToCreateWidenRecipe(
if (Phi->getParent() != OrigLoop->getHeader())
return tryToBlend(Phi, Operands, Plan);
- // Always record recipes for header phis. Later first-order recurrence phis
- // can have earlier phis as incoming values.
- recordRecipeOf(Phi);
-
if ((Recipe = tryToOptimizeInductionPHI(Phi, Operands, *Plan, Range)))
return Recipe;
@@ -8445,14 +8453,6 @@ VPRecipeBase *VPRecipeBuilder::tryToCreateWidenRecipe(
PhiRecipe = new VPFirstOrderRecurrencePHIRecipe(Phi, *StartV);
}
- // Record the incoming value from the backedge, so we can add the incoming
- // value from the backedge after all recipes have been created.
- auto *Inc = cast<Instruction>(
- Phi->getIncomingValueForBlock(OrigLoop->getLoopLatch()));
- auto RecipeIter = Ingredient2Recipe.find(Inc);
- if (RecipeIter == Ingredient2Recipe.end())
- recordRecipeOf(Inc);
-
PhisToFix.push_back(PhiRecipe);
return PhiRecipe;
}
@@ -8518,7 +8518,7 @@ void LoopVectorizationPlanner::buildVPlansWithVPRecipes(ElementCount MinVF,
static void addCanonicalIVRecipes(VPlan &Plan, Type *IdxTy, bool HasNUW,
DebugLoc DL) {
Value *StartIdx = ConstantInt::get(IdxTy, 0);
- auto *StartV = Plan.getVPValueOrAddLiveIn(StartIdx);
+ auto *StartV = Plan.getOrAddLiveIn(StartIdx);
// Add a VPCanonicalIVPHIRecipe starting at 0 to the header.
auto *CanonicalIVPHI = new VPCanonicalIVPHIRecipe(StartV, DL);
@@ -8541,7 +8541,7 @@ static void addCanonicalIVRecipes(VPlan &Plan, Type *IdxTy, bool HasNUW,
// Add exit values to \p Plan. VPLiveOuts are added for each LCSSA phi in the
// original exit block.
static void addUsersInExitBlock(VPBasicBlock *HeaderVPBB, Loop *OrigLoop,
- VPlan &Plan) {
+ VPRecipeBuilder &Builder, VPlan &Plan) {
BasicBlock *ExitBB = OrigLoop->getUniqueExitBlock();
BasicBlock *ExitingBB = OrigLoop->getExitingBlock();
// Only handle single-exit loops with unique exit blocks for now.
@@ -8552,7 +8552,7 @@ static void addUsersInExitBlock(VPBasicBlock *HeaderVPBB, Loop *OrigLoop,
for (PHINode &ExitPhi : ExitBB->phis()) {
Value *IncomingValue =
ExitPhi.getIncomingValueForBlock(ExitingBB);
- VPValue *V = Plan.getVPValueOrAddLiveIn(IncomingValue);
+ VPValue *V = Builder.getVPValue(IncomingValue, Plan);
Plan.addLiveOut(&ExitPhi, V);
}
}
@@ -8588,9 +8588,6 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
if (!getDecisionAndClampRange(applyIG, Range))
continue;
InterleaveGroups.insert(IG);
- for (unsigned i = 0; i < IG->getFactor(); i++)
- if (Instruction *Member = IG->getMember(i))
- RecipeBuilder.recordRecipeOf(Member);
};
// ---------------------------------------------------------------------------
@@ -8659,10 +8656,10 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
SmallVector<VPValue *, 4> Operands;
auto *Phi = dyn_cast<PHINode>(Instr);
if (Phi && Phi->getParent() == HeaderBB) {
- Operands.push_back(Plan->getVPValueOrAddLiveIn(
+ Operands.push_back(Plan->getOrAddLiveIn(
Phi->getIncomingValueForBlock(OrigLoop->getLoopPreheader())));
} else {
- auto OpRange = Plan->mapToVPValues(Instr->operands());
+ auto OpRange = RecipeBuilder.mapToVPValues(Instr->operands(), *Plan);
Operands = {OpRange.begin(), OpRange.end()};
}
@@ -8677,10 +8674,6 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
Instr, Operands, Range, VPBB, Plan);
if (!Recipe)
Recipe = RecipeBuilder.handleReplication(Instr, Range, *Plan);
- for (auto *Def : Recipe->definedValues()) {
- auto *UV = Def->getUnderlyingValue();
- Plan->addVPValue(UV, Def);
- }
RecipeBuilder.setRecipe(Instr, Recipe);
if (isa<VPHeaderPHIRecipe>(Recipe)) {
@@ -8712,7 +8705,7 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
// and there is nothing to fix from vector loop; phis should have incoming
// from scalar loop only.
} else
- addUsersInExitBlock(HeaderVPBB, OrigLoop, *Plan);
+ addUsersInExitBlock(HeaderVPBB, OrigLoop, RecipeBuilder, *Plan);
assert(isa<VPRegionBlock>(Plan->getVectorLoopRegion()) &&
!Plan->getVectorLoopRegion()->getEntryBasicBlock()->empty() &&
@@ -8774,16 +8767,12 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
continue;
Constant *CI = ConstantInt::get(Stride->getType(), ScevStride->getAPInt());
- auto *ConstVPV = Plan->getVPValueOrAddLiveIn(CI);
+ auto *ConstVPV = Plan->getOrAddLiveIn(CI);
// The versioned value may not be used in the loop directly, so just add a
// new live-in in those cases.
- Plan->getVPValueOrAddLiveIn(StrideV)->replaceAllUsesWith(ConstVPV);
+ Plan->getOrAddLiveIn(StrideV)->replaceAllUsesWith(ConstVPV);
}
- // From this point onwards, VPlan-to-VPlan transformations may change the plan
- // in ways that accessing values using original IR values is incorrect.
- Plan->disableValue2VPValue();
-
VPlanTransforms::dropPoisonGeneratingRecipes(*Plan, [this](BasicBlock *BB) {
return Legal->blockNeedsPredication(BB);
});
@@ -10082,7 +10071,7 @@ bool LoopVectorizePass::processLoop(Loop *L) {
EpilogILV.setTripCount(MainILV.getTripCount());
for (auto &R : make_early_inc_range(*BestEpiPlan.getPreheader())) {
auto *ExpandR = cast<VPExpandSCEVRecipe>(&R);
- auto *ExpandedVal = BestEpiPlan.getVPValueOrAddLiveIn(
+ auto *ExpandedVal = BestEpiPlan.getOrAddLiveIn(
ExpandedSCEVs.find(ExpandR->getSCEV())->second);
ExpandR->replaceAllUsesWith(ExpandedVal);
if (BestEpiPlan.getTripCount() == ExpandR)
@@ -10123,7 +10112,7 @@ bool LoopVectorizePass::processLoop(Loop *L) {
{EPI.MainLoopIterationCountCheck});
}
assert(ResumeV && "Must have a resume value");
- VPValue *StartVal = BestEpiPlan.getVPValueOrAddLiveIn(ResumeV);
+ VPValue *StartVal = BestEpiPlan.getOrAddLiveIn(ResumeV);
cast<VPHeaderPHIRecipe>(&R)->setStartValue(StartVal);
}
diff --git a/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h b/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
index b1498026adadfe..527417e29906fe 100644
--- a/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
+++ b/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
@@ -49,9 +49,8 @@ class VPRecipeBuilder {
EdgeMaskCacheTy EdgeMaskCache;
BlockMaskCacheTy BlockMaskCache;
- // VPlan-VPlan transformations support: Hold a mapping from ingredients to
- // their recipe. To save on memory, only do so for selected ingredients,
- // marked by having a nullptr entry in this map.
+ // VPlan construction support: Hold a mapping from ingredients to
+ // their recipe.
DenseMap<Instruction *, VPRecipeBase *> Ingredient2Recipe;
/// Cross-iteration reduction & first-order recurrence phis for which we need
@@ -117,13 +116,8 @@ class VPRecipeBuilder {
VFRange &Range, VPBasicBlock *VPBB,
VPlanPtr &Plan);
- /// Set the recipe created for given ingredient. This operation is a no-op for
- /// ingredients that were not marked using a nullptr entry in the map.
+ /// Set the recipe created for given ingredient.
void setRecipe(Instruction *I, VPRecipeBase *R) {
- if (!Ingredient2Recipe.count(I))
- return;
- assert(Ingredient2Recipe[I] == nullptr &&
- "Recipe already set for ingredient");
Ingredient2Recipe[I] = R;
}
@@ -146,14 +140,6 @@ class VPRecipeBuilder {
/// between SRC and DST.
VPValue *getEdgeMask(BasicBlock *Src, BasicBlock *Dst) const;
- /// Mark given ingredient for recording its recipe once one is created for
- /// it.
- void recordRecipeOf(Instruction *I) {
- assert((!Ingredient2Recipe.count(I) || Ingredient2Recipe[I] == nullptr) &&
- "Recipe already set for ingredient");
- Ingredient2Recipe[I] = nullptr;
- }
-
/// Return the recipe created for given ingredient.
VPRecipeBase *getRecipe(Instruction *I) {
assert(Ingredient2Recipe.count(I) &&
@@ -172,6 +158,19 @@ class VPRecipeBuilder {
/// Add the incoming values from the backedge to reduction & first-order
/// recurrence cross-iteration phis.
void fixHeaderPhis();
+
+ /// Returns a range mapping the values the range \p Operands to their
+ /// corresponding VPValues.
+ iterator_range<mapped_iterator<Use *, std::function<VPValue *(Value *)>>>
+ mapToVPValues(User::op_range Operands, VPlan &Plan);
+
+ VPValue *getVPValue(Value *V, VPlan &Plan) {
+ if (auto *I = dyn_cast<Instruction>(V)) {
+ if (auto *R = Ingredient2Recipe.lookup(I))
+ return R->getVPSingleValue();
+ }
+ return Plan.getOrAddLiveIn(V);
+ }
};
} // end namespace llvm
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.cpp b/llvm/lib/Transforms/Vectorize/VPlan.cpp
index 9768e4b7aa0a8a..53ee9793fdefe9 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlan.cpp
@@ -812,7 +812,7 @@ void VPlan::prepareToExecute(Value *TripCountV, Value *VectorTripCountV,
// needs to be changed from zero to the value after the main vector loop.
// FIXME: Improve modeling for canonical IV start values in the epilogue loop.
if (CanonicalIVStartValue) {
- VPValue *VPV = getVPValueOrAddLiveIn(CanonicalIVStartValue);
+ VPValue *VPV = getOrAddLiveIn(CanonicalIVStartValue);
auto *IV = getCanonicalIV();
assert(all_of(IV->users(),
[](const VPUser *U) {
@@ -1091,7 +1091,7 @@ VPlan *VPlan::duplicate() {
DenseMap<VPValue *, VPValue *> Old2NewVPValues;
for (VPValue *OldLiveIn : VPLiveInsToFree) {
Old2NewVPValues[OldLiveIn] =
- NewPlan->getVPValueOrAddLiveIn(OldLiveIn->getLiveInIRValue());
+ NewPlan->getOrAddLiveIn(OldLiveIn->getLiveInIRValue());
}
Old2NewVPValues[&VectorTripCount] = &NewPlan->VectorTripCount;
Old2NewVPValues[&VFxUF] = &NewPlan->VFxUF;
@@ -1102,7 +1102,7 @@ VPlan *VPlan::duplicate() {
assert(TripCount && "trip count must be set");
if (TripCount->isLiveIn())
Old2NewVPValues[TripCount] =
- NewPlan->getVPValueOrAddLiveIn(TripCount->getLiveInIRValue());
+ NewPlan->getOrAddLiveIn(TripCount->getLiveInIRValue());
// else NewTripCount will be created and inserted into Old2NewVPValues when
// TripCount is cloned. In any case NewPlan->TripCount is updated below.
@@ -1425,9 +1425,9 @@ VPValue *vputils::getOrCreateVPValueForSCEVExpr(VPlan &Plan, const SCEV *Expr,
return Expanded;
VPValue *Expanded = nullptr;
if (auto *E = dyn_cast<SCEVConstant>(Expr))
- Expanded = Plan.getVPValueOrAddLiveIn(E->getValue());
+ Expanded = Plan.getOrAddLiveIn(E->getValue());
else if (auto *E = dyn_cast<SCEVUnknown>(Expr))
- Expanded = Plan.getVPValueOrAddLiveIn(E->getValue());
+ Expanded = Plan.getOrAddLiveIn(E->getValue());
else {
Expanded = new VPExpandSCEVRecipe(Expr, SE);
Plan.getPreheader()->appendRecipe(Expanded->getDefiningRecipe());
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index af6d0081bffebc..26162c25664e5b 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -2870,10 +2870,6 @@ class VPlan {
/// definitions are VPValues that hold a pointer to their underlying IR.
SmallVector<VPValue *, 16> VPLiveInsToFree;
- /// Indicates whether it is safe use the Value2VPValue mapping or if the
- /// mapping cannot be used any longer, because it is stale.
- bool Value2VPValueEnabled = true;
-
/// Values used outside the plan.
MapVector<PHINode *, VPLiveOut *> LiveOuts;
@@ -2952,10 +2948,6 @@ class VPlan {
/// Returns VF * UF of the vector loop region.
VPValue &getVFxUF() { return VFxUF; }
- /// Mark the plan to indicate that using Value2VPValue is not safe any
- /// longer, because it may be stale.
- void disableValue2VPValue() { Value2VPValueEnabled = false; }
-
void addVF(ElementCount VF) { VFs.insert(VF); }
void setVF(ElementCount VF) {
@@ -2985,25 +2977,22 @@ class VPlan {
void setName(const Twine &newName) { Name = newName.str(); }
void addVPValue(Value *V, VPValue *VPV) {
- assert((Value2VPValueEnabled || VPV->isLiveIn()) &&
- "Value2VPValue mapping may be out of date!");
- assert(V && "Trying to add a null Value to VPlan");
- assert(!Value2VPValue.count(V) && "Value already exists in VPlan");
+ assert(VPV->isLiveIn());
Value2VPValue[V] = VPV;
}
/// Returns the VPValue for \p V.
- VPValue *getVPValue(Value *V) {
+ VPValue *getLiveIn(Value *V) {
assert(V && "Trying to get the VPValue of a null Value");
assert(Value2VPValue.count(V) && "Value does not exist in VPlan");
- assert((Value2VPValueEnabled || Value2VPValue[V]->isLiveIn()) &&
- "Value2VPValue mapping may be out of date!");
+ assert(Value2VPValue[V]->isLiveIn() &&
+ "Only live-ins should be in mapping");
return Value2VPValue[V];
}
/// Gets the VPValue for \p V or adds a new live-in (if none exists yet) for
/// \p V.
- VPValue *getVPValueOrAddLiveIn(Value *V) {
+ VPValue *getOrAddLiveIn(Value *V) {
assert(V && "Trying to get or add the VPValue of a null Value");
if (!Value2VPValue.count(V)) {
VPValue *VPV = new VPValue(V);
@@ -3011,7 +3000,7 @@ class VPlan {
addVPValue(V, VPV);
}
- return getVPValue(V);
+ return getLiveIn(V);
}
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
@@ -3028,16 +3017,6 @@ class VPlan {
LLVM_DUMP_METHOD void dump() const;
#endif
- /// Returns a range mapping the values the range \p Operands to their
- /// corresponding VPValues.
- iterator_range<mapped_iterator<Use *, std::function<VPValue *(Value *)>>>
- mapToVPValues(User::op_range Operands) {
- std::function<VPValue *(Value *)> Fn = [this](Value *Op) {
- return getVPValueOrAddLiveIn(Op);
- };
- return map_range(Operands, Fn);
- }
-
/// Returns the VPRegionBlock of the vector loop.
VPRegionBlock *getVectorLoopRegion() {
return cast<VPRegionBlock>(getEntry()->getSingleSuccessor());
diff --git a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
index 6474a9697dce89..da246869625bb0 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
@@ -272,7 +272,7 @@ VPValue *PlainCFGBuilder::getOrCreateVPOperand(Value *IRVal) {
// A and B: Create VPValue and add it to the pool of external definitions and
// to the Value->VPValue map.
- VPValue *NewVPVal = Plan.getVPValueOrAddLiveIn(IRVal);
+ VPValue *NewVPVal = Plan.getOrAddLiveIn(IRVal);
IRDef2VPValue[IRVal] = NewVPVal;
return NewVPVal;
}
@@ -362,7 +362,7 @@ void PlainCFGBuilder::buildPlainCFG() {
for (auto &I : *ThePreheaderBB) {
if (I.getType()->isVoidTy())
continue;
- IRDef2VPValue[&I] = Plan.getVPValueOrAddLiveIn(&I);
+ IRDef2VPValue[&I] = Plan.getOrAddLiveIn(&I);
}
LoopBlocksRPO RPO(TheLoop);
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
index f6b564ad931ca9..93cbe7065db572 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -49,12 +49,11 @@ void VPlanTransforms::VPInstructionsToVPRecipes(
if (auto *VPPhi = dyn_cast<VPWidenPHIRecipe>(&Ingredient)) {
auto *Phi = cast<PHINode>(VPPhi->getUnderlyingValue());
if (const auto *II = GetIntOrFpInductionDescriptor(Phi)) {
- VPValue *Start = Plan->getVPValueOrAddLiveIn(II->getStartValue());
+ VPValue *Start = Plan->getOrAddLiveIn(II->getStartValue());
VPValue *Step =
vputils::getOrCreateVPValueForSCEVExpr(*Plan, II->getStep(), SE);
NewRecipe = new VPWidenIntOrFpInductionRecipe(Phi, Start, Step, *II);
} else {
- Plan->addVPValue(Phi, VPPhi);
continue;
}
} else {
@@ -625,9 +624,9 @@ void VPlanTransforms::optimizeForVFAndUF(VPlan &Plan, ElementCount BestVF,
return;
LLVMContext &Ctx = SE.getContext();
- ...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Nice clean-up! Conceptually analogous to TransformState which keeps track of VPValue-to-Value(s) mapping during codegen only. Independent of this patch: should VPRecipeBuilder (or some other sub/class thereof) be confined to initial VPlan construction from LLVM-IR, excluding construction of recipes during VPlan-to-VPlan transformations? |
ayalz
reviewed
Mar 11, 2024
| @@ -172,6 +158,19 @@ class VPRecipeBuilder { | |||
| /// Add the incoming values from the backedge to reduction & first-order | |||
| /// recurrence cross-iteration phis. | |||
| void fixHeaderPhis(); | |||
|
|
|||
| /// Returns a range mapping the values the range \p Operands to their | |||
There was a problem hiding this comment.
Suggested change
| /// Returns a range mapping the values the range \p Operands to their | |
| /// Returns a range mapping the values of the range \p Operands to their |
(admittedly fixing an existing typo)
| iterator_range<mapped_iterator<Use *, std::function<VPValue *(Value *)>>> | ||
| mapToVPValues(User::op_range Operands, VPlan &Plan); | ||
|
|
||
| VPValue *getVPValue(Value *V, VPlan &Plan) { |
| "Value2VPValue mapping may be out of date!"); | ||
| assert(V && "Trying to add a null Value to VPlan"); | ||
| assert(!Value2VPValue.count(V) && "Value already exists in VPlan"); | ||
| assert(VPV->isLiveIn()); |
There was a problem hiding this comment.
added, thanks, also changed to addLiveIn
Comment on lines
3006
to
2995
| VPValue *getVPValueOrAddLiveIn(Value *V) { | ||
| VPValue *getOrAddLiveIn(Value *V) { |
There was a problem hiding this comment.
Why drop VPValue prefix? If desired, can be pushed separately?
There was a problem hiding this comment.
I think it is more accurate (and compact), but I removed that change, to reduce the diff
There was a problem hiding this comment.
Certainly more accurate (LiveIn is also a VPValue) and compact. Worth pushing independently.
| @@ -117,13 +116,8 @@ class VPRecipeBuilder { | |||
| VFRange &Range, VPBasicBlock *VPBB, | |||
| VPlanPtr &Plan); | |||
|
|
|||
| /// Set the recipe created for given ingredient. This operation is a no-op for | |||
| /// ingredients that were not marked using a nullptr entry in the map. | |||
| /// Set the recipe created for given ingredient. | |||
| void setRecipe(Instruction *I, VPRecipeBase *R) { | |||
There was a problem hiding this comment.
Worth asserting to avoid resetting an already set recipe?
There was a problem hiding this comment.
Added assert, thanks!
Comment on lines
2990
to
2993
| assert(V && "Trying to add a null Value to VPlan"); | ||
| assert(!Value2VPValue.count(V) && "Value already exists in VPlan"); |
There was a problem hiding this comment.
Worth retaining these asserts?
…value-mapping-for-non-live-outs
fhahn
commented
Mar 11, 2024
There was a problem hiding this comment.
Independent of this patch: should VPRecipeBuilder (or some other sub/class thereof) be confined to initial VPlan construction from LLVM-IR, excluding construction of recipes during VPlan-to-VPlan transformations?
Yes, VPRecipeBuilder ( not to be confused with VPBuilder) is only used for initial construction. We should split of the logic to create the initial VPlan to a separate helper, and only instantiate VPRecipeBuilder there
| @@ -117,13 +116,8 @@ class VPRecipeBuilder { | |||
| VFRange &Range, VPBasicBlock *VPBB, | |||
| VPlanPtr &Plan); | |||
|
|
|||
| /// Set the recipe created for given ingredient. This operation is a no-op for | |||
| /// ingredients that were not marked using a nullptr entry in the map. | |||
| /// Set the recipe created for given ingredient. | |||
| void setRecipe(Instruction *I, VPRecipeBase *R) { | |||
There was a problem hiding this comment.
Added assert, thanks!
| @@ -172,6 +158,19 @@ class VPRecipeBuilder { | |||
| /// Add the incoming values from the backedge to reduction & first-order | |||
| /// recurrence cross-iteration phis. | |||
| void fixHeaderPhis(); | |||
|
|
|||
| /// Returns a range mapping the values the range \p Operands to their | |||
| iterator_range<mapped_iterator<Use *, std::function<VPValue *(Value *)>>> | ||
| mapToVPValues(User::op_range Operands, VPlan &Plan); | ||
|
|
||
| VPValue *getVPValue(Value *V, VPlan &Plan) { |
| "Value2VPValue mapping may be out of date!"); | ||
| assert(V && "Trying to add a null Value to VPlan"); | ||
| assert(!Value2VPValue.count(V) && "Value already exists in VPlan"); | ||
| assert(VPV->isLiveIn()); |
There was a problem hiding this comment.
added, thanks, also changed to addLiveIn
Comment on lines
2990
to
2993
| assert(V && "Trying to add a null Value to VPlan"); | ||
| assert(!Value2VPValue.count(V) && "Value already exists in VPlan"); |
Comment on lines
3006
to
2995
| VPValue *getVPValueOrAddLiveIn(Value *V) { | ||
| VPValue *getOrAddLiveIn(Value *V) { |
There was a problem hiding this comment.
I think it is more accurate (and compact), but I removed that change, to reduce the diff
| @@ -7938,7 +7950,7 @@ VPValue *VPRecipeBuilder::createEdgeMask(BasicBlock *Src, BasicBlock *Dst, | |||
| if (OrigLoop->isLoopExiting(Src)) | |||
| return EdgeMaskCache[Edge] = SrcMask; | |||
|
|
|||
| VPValue *EdgeMask = Plan.getVPValueOrAddLiveIn(BI->getCondition()); | |||
| VPValue *EdgeMask = getVPValueOrAddLiveIn(BI->getCondition(), Plan); | |||
There was a problem hiding this comment.
Does it make more sense to move VPlan to VPRecipeBuilder ?
There was a problem hiding this comment.
Forgot to mention, this has been split off & landed in 8578b6e, thanks!
fhahn
added a commit
that referenced
this pull request
Mar 18, 2024
Instead of passing VPlan in a number of places, just store it directly in VPRecipeBuilder. A single instance is only used for a single VPlan. This simplifies the code and was suggested by @nikolaypanchenko in #84464.
…value-mapping-for-non-live-outs
Agreed. Would also be good to clarify the distinction between the two builders, possibly using more accurate names. May also be interesting to consider the analogous TransformState, used to maintain the temporary state during translation from VPlan to LLVM-IR. |
ayalz
approved these changes
Mar 22, 2024
| @@ -54,7 +54,6 @@ void VPlanTransforms::VPInstructionsToVPRecipes( | |||
| vputils::getOrCreateVPValueForSCEVExpr(*Plan, II->getStep(), SE); | |||
| NewRecipe = new VPWidenIntOrFpInductionRecipe(Phi, Start, Step, *II); | |||
| } else { | |||
| Plan->addVPValue(Phi, VPPhi); | |||
There was a problem hiding this comment.
nit: now more appealing to swap and have an early continue.
Comment on lines
3006
to
2995
| VPValue *getVPValueOrAddLiveIn(Value *V) { | ||
| VPValue *getOrAddLiveIn(Value *V) { |
There was a problem hiding this comment.
Certainly more accurate (LiveIn is also a VPValue) and compact. Worth pushing independently.
chencha3
pushed a commit
to chencha3/llvm-project
that referenced
this pull request
Mar 23, 2024
Instead of passing VPlan in a number of places, just store it directly in VPRecipeBuilder. A single instance is only used for a single VPlan. This simplifies the code and was suggested by @nikolaypanchenko in llvm#84464.
…value-mapping-for-non-live-outs
|
✅ With the latest revision this PR passed the Python code formatter. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of keeping a mapping of Inst->VPValues (of their corresponding recipes) in VPlan's Value2VPValue mapping, keep it in VPRecipeBuilder instead. After recently replacing the last user of this mapping after initial construction, this mapping is only needed for recipe construction (to map IR operands to VPValue operands).
By moving the mapping, VPlan's VPValue tracking can be simplified and limited only to live-ins. It also allows removing disableValue2VPValue and associated machinery & asserts.