[VPlan] Version VPValue names in VPSlotTracker. #81411
Conversation
This patch restructures the way names for printing VPValues are handled. It updates VPlan to track assigned names for VPValues (similar to how LLVMContext tracks names for LLVM IR Values). Tracking names in a single place allows to deduplicate names and ensure all names are unique. Now there are the following cases: * If a VPValue has a defining recipe, its name is determined by checking its associated VPlan's mapping. The name is wrapped in vp<>. If no VPlan is associated, <badref> is used. * If a VPValue is a live-in with an underlying IR value, the name of the IR value is used and wrapped in ir<>. * If none of the above applies, the number assigned by VPSlotTracker is used wrapped in vp<> if an associated VPlan is provided or <badref> otherwise. Note that contrary to LLMV's Value which always can retrieve LLVMContext, this is not possible for VPValues without defining recipe, so in some cases, the associated VPlan needs to be provided explicitly. The new handling subsumes VPInstruction's manual handling of its name.
|
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-llvm-transforms Author: Florian Hahn (fhahn) ChangesThis patch restructures the way names for printing VPValues are handled. It updates VPlan to track assigned names for VPValues (similar to how LLVMContext tracks names for LLVM IR Values). Tracking names in a single place allows to deduplicate names and ensure all names are unique. Now there are the following cases:
Note that contrary to LLMV's Value which always can retrieve LLVMContext, this is not possible for VPValues without defining recipe, so in some cases, the associated VPlan needs to be provided explicitly. The new handling subsumes VPInstruction's manual handling of its name. Patch is 133.80 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/81411.diff 24 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h b/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
index a7ebf78e54ceb6..5aa4892bf0ffe5 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
@@ -47,16 +47,19 @@ class VPBuilder {
VPBasicBlock::iterator InsertPt = VPBasicBlock::iterator();
/// Insert \p VPI in BB at InsertPt if BB is set.
- VPInstruction *tryInsertInstruction(VPInstruction *VPI) {
- if (BB)
+ VPInstruction *tryInsertInstruction(VPInstruction *VPI,
+ const Twine &Name = "") {
+ if (BB) {
BB->insert(VPI, InsertPt);
+ VPI->setName(Name);
+ }
return VPI;
}
VPInstruction *createInstruction(unsigned Opcode,
ArrayRef<VPValue *> Operands, DebugLoc DL,
const Twine &Name = "") {
- return tryInsertInstruction(new VPInstruction(Opcode, Operands, DL, Name));
+ return tryInsertInstruction(new VPInstruction(Opcode, Operands, DL), Name);
}
VPInstruction *createInstruction(unsigned Opcode,
@@ -150,7 +153,7 @@ class VPBuilder {
VPRecipeWithIRFlags::WrapFlagsTy WrapFlags,
DebugLoc DL = {}, const Twine &Name = "") {
return tryInsertInstruction(
- new VPInstruction(Opcode, Operands, WrapFlags, DL, Name));
+ new VPInstruction(Opcode, Operands, WrapFlags, DL), Name);
}
VPValue *createNot(VPValue *Operand, DebugLoc DL = {},
const Twine &Name = "") {
@@ -170,12 +173,12 @@ class VPBuilder {
VPValue *createSelect(VPValue *Cond, VPValue *TrueVal, VPValue *FalseVal,
DebugLoc DL = {}, const Twine &Name = "",
std::optional<FastMathFlags> FMFs = std::nullopt) {
- auto *Select =
- FMFs ? new VPInstruction(Instruction::Select, {Cond, TrueVal, FalseVal},
- *FMFs, DL, Name)
- : new VPInstruction(Instruction::Select, {Cond, TrueVal, FalseVal},
- DL, Name);
- return tryInsertInstruction(Select);
+ auto *Select = FMFs
+ ? new VPInstruction(Instruction::Select,
+ {Cond, TrueVal, FalseVal}, *FMFs, DL)
+ : new VPInstruction(Instruction::Select,
+ {Cond, TrueVal, FalseVal}, DL);
+ return tryInsertInstruction(Select, Name);
}
/// Create a new ICmp VPInstruction with predicate \p Pred and operands \p A
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 1a7b301c35f2b8..d08aa8fcb12c24 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -7262,7 +7262,7 @@ VPValue *VPBuilder::createICmp(CmpInst::Predicate Pred, VPValue *A, VPValue *B,
assert(Pred >= CmpInst::FIRST_ICMP_PREDICATE &&
Pred <= CmpInst::LAST_ICMP_PREDICATE && "invalid predicate");
return tryInsertInstruction(
- new VPInstruction(Instruction::ICmp, Pred, A, B, DL, Name));
+ new VPInstruction(Instruction::ICmp, Pred, A, B, DL), Name);
}
// This function will select a scalable VF if the target supports scalable
@@ -8610,11 +8610,11 @@ static void addCanonicalIVRecipes(VPlan &Plan, Type *IdxTy, bool HasNUW,
// IV by VF * UF.
auto *CanonicalIVIncrement =
new VPInstruction(Instruction::Add, {CanonicalIVPHI, &Plan.getVFxUF()},
- {HasNUW, false}, DL, "index.next");
+ {HasNUW, false}, DL);
CanonicalIVPHI->addOperand(CanonicalIVIncrement);
VPBasicBlock *EB = TopRegion->getExitingBasicBlock();
- EB->appendRecipe(CanonicalIVIncrement);
+ EB->appendRecipe(CanonicalIVIncrement, "index.next");
// Add the BranchOnCount VPInstruction to the latch.
VPInstruction *BranchBack =
@@ -8781,8 +8781,9 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
CM.foldTailByMasking() || isa<TruncInst>(Instr)) &&
"unexpected recipe needs moving");
Recipe->insertBefore(*HeaderVPBB, HeaderVPBB->getFirstNonPhi());
+ Recipe->getVPSingleValue()->setName(Instr->getName());
} else
- VPBB->appendRecipe(Recipe);
+ VPBB->appendRecipe(Recipe, Instr->getName());
}
VPBlockUtils::insertBlockAfter(new VPBasicBlock(), VPBB);
@@ -8842,6 +8843,8 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
J++;
}
MemberR->eraseFromParent();
+ if (!Member->getType()->isVoidTy())
+ VPIG->getVPValue(J - 1)->setName(Member->getName());
}
}
@@ -9097,7 +9100,7 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
// ensure that it comes after all of it's inputs, including CondOp.
// Note that this transformation may leave over dead recipes (including
// CurrentLink), which will be cleaned by a later VPlan transform.
- LinkVPBB->appendRecipe(RedRecipe);
+ LinkVPBB->appendRecipe(RedRecipe, CurrentLinkI->getName());
CurrentLink->replaceAllUsesWith(RedRecipe);
PreviousLink = RedRecipe;
}
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.cpp b/llvm/lib/Transforms/Vectorize/VPlan.cpp
index 2c0daa82afa59f..bb603653f37ddc 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlan.cpp
@@ -588,6 +588,15 @@ bool VPBasicBlock::isExiting() const {
return getParent()->getExitingBasicBlock() == this;
}
+void VPBasicBlock::appendRecipe(VPRecipeBase *Recipe, const Twine &Name) {
+ insert(Recipe, end());
+ if (Recipe->getNumDefinedValues() == 1)
+ Recipe->getVPSingleValue()->setName(Name);
+ else
+ assert(Name.str().empty() && "Provided a name for a recipe that does not "
+ "define exactly a single VPValue");
+}
+
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
void VPBlockBase::printSuccessors(raw_ostream &O, const Twine &Indent) const {
if (getSuccessors().empty()) {
@@ -1053,8 +1062,10 @@ static void remapOperands(VPBlockBase *Entry, VPBlockBase *NewEntry,
assert(OldR.getNumDefinedValues() == NewR.getNumDefinedValues() &&
"recipes must define the same number of operands");
for (const auto &[OldV, NewV] :
- zip(OldR.definedValues(), NewR.definedValues()))
+ zip(OldR.definedValues(), NewR.definedValues())) {
Old2NewVPValues[OldV] = NewV;
+ NewV->setName(OldV->getName());
+ }
}
}
@@ -1112,6 +1123,26 @@ VPlan *VPlan::duplicate() {
return NewPlan;
}
+void VPlan::setName(const VPValue *V, const Twine &Name) {
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+ std::string N = Name.str();
+ if (N.empty())
+ return;
+ assert(!VPValue2Name.contains(V));
+ std::string CurrName = N;
+
+ if (UsedNames.contains(N))
+ CurrName = N + ".1";
+ unsigned Cnt = 2;
+ while (UsedNames.contains(CurrName)) {
+ CurrName = N + "." + std::to_string(Cnt);
+ Cnt += 1;
+ }
+ VPValue2Name[V] = CurrName;
+ UsedNames.insert(CurrName);
+#endif
+}
+
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
Twine VPlanPrinter::getUID(const VPBlockBase *Block) {
@@ -1298,18 +1329,12 @@ void VPValue::replaceUsesWithIf(
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
void VPValue::printAsOperand(raw_ostream &OS, VPSlotTracker &Tracker) const {
- if (const Value *UV = getUnderlyingValue()) {
+ if (!getDefiningRecipe() && getUnderlyingValue()) {
OS << "ir<";
- UV->printAsOperand(OS, false);
+ getUnderlyingValue()->printAsOperand(OS, false);
OS << ">";
- return;
- }
-
- unsigned Slot = Tracker.getSlot(this);
- if (Slot == unsigned(-1))
- OS << "<badref>";
- else
- OS << "vp<%" << Tracker.getSlot(this) << ">";
+ } else
+ OS << Tracker.getName(this);
}
void VPUser::printOperands(raw_ostream &O, VPSlotTracker &SlotTracker) const {
@@ -1319,6 +1344,31 @@ void VPUser::printOperands(raw_ostream &O, VPSlotTracker &SlotTracker) const {
}
#endif
+std::string VPValue::getName() {
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+ if (auto *Def = getDefiningRecipe())
+ return Def->getParent()->getPlan()->getName(this);
+ assert(isLiveIn() && "called getName() on unsupported VPValue");
+ return getLiveInIRValue()->getName().str();
+#else
+ return "";
+#endif
+}
+
+void VPValue::setName(const Twine &Name) {
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+ auto *Def = getDefiningRecipe();
+ Def->getParent()->getPlan()->setName(this, Name);
+#endif
+}
+
+void VPValue::takeName(const VPValue *Src) {
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+ auto *Def = getDefiningRecipe();
+ Def->getParent()->getPlan()->takeName(Src, this);
+#endif
+}
+
void VPInterleavedAccessInfo::visitRegion(VPRegionBlock *Region,
Old2NewTy &Old2New,
InterleavedAccessInfo &IAI) {
@@ -1397,6 +1447,16 @@ void VPSlotTracker::assignSlots(const VPBasicBlock *VPBB) {
assignSlot(Def);
}
+std::string VPSlotTracker::getName(const VPValue *V) const {
+ if (!Plan)
+ return "<badref>";
+ std::string N = Plan->getName(V);
+ if (!N.empty())
+ return (Twine("vp<%") + N + ">").str();
+
+ return (Twine("vp<%") + std::to_string(getSlot(V)) + ">").str();
+}
+
bool vputils::onlyFirstLaneUsed(const VPValue *Def) {
return all_of(Def->users(),
[Def](const VPUser *U) { return U->onlyFirstLaneUsed(Def); });
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 162a3c4b195e53..08c0041f23ae07 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -32,6 +32,7 @@
#include "llvm/ADT/SmallBitVector.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringSet.h"
#include "llvm/ADT/Twine.h"
#include "llvm/ADT/ilist.h"
#include "llvm/ADT/ilist_node.h"
@@ -1162,9 +1163,6 @@ class VPInstruction : public VPRecipeWithIRFlags {
typedef unsigned char OpcodeTy;
OpcodeTy Opcode;
- /// An optional name that can be used for the generated IR instruction.
- const std::string Name;
-
/// Utility method serving execute(): generates a single instance of the
/// modeled instruction. \returns the generated value for \p Part.
/// In some cases an existing value is returned rather than a generated
@@ -1181,31 +1179,30 @@ class VPInstruction : public VPRecipeWithIRFlags {
void setUnderlyingInstr(Instruction *I) { setUnderlyingValue(I); }
public:
- VPInstruction(unsigned Opcode, ArrayRef<VPValue *> Operands, DebugLoc DL,
- const Twine &Name = "")
+ VPInstruction(unsigned Opcode, ArrayRef<VPValue *> Operands, DebugLoc DL)
: VPRecipeWithIRFlags(VPDef::VPInstructionSC, Operands, DL),
- Opcode(Opcode), Name(Name.str()) {}
+ Opcode(Opcode) {}
VPInstruction(unsigned Opcode, std::initializer_list<VPValue *> Operands,
- DebugLoc DL = {}, const Twine &Name = "")
- : VPInstruction(Opcode, ArrayRef<VPValue *>(Operands), DL, Name) {}
+ DebugLoc DL = {})
+ : VPInstruction(Opcode, ArrayRef<VPValue *>(Operands), DL) {}
VPInstruction(unsigned Opcode, CmpInst::Predicate Pred, VPValue *A,
- VPValue *B, DebugLoc DL = {}, const Twine &Name = "");
+ VPValue *B, DebugLoc DL = {});
VPInstruction(unsigned Opcode, std::initializer_list<VPValue *> Operands,
- WrapFlagsTy WrapFlags, DebugLoc DL = {}, const Twine &Name = "")
+ WrapFlagsTy WrapFlags, DebugLoc DL = {})
: VPRecipeWithIRFlags(VPDef::VPInstructionSC, Operands, WrapFlags, DL),
- Opcode(Opcode), Name(Name.str()) {}
+ Opcode(Opcode) {}
VPInstruction(unsigned Opcode, std::initializer_list<VPValue *> Operands,
- FastMathFlags FMFs, DebugLoc DL = {}, const Twine &Name = "");
+ FastMathFlags FMFs, DebugLoc DL = {});
VP_CLASSOF_IMPL(VPDef::VPInstructionSC)
VPRecipeBase *clone() override {
SmallVector<VPValue *, 2> Operands(operands());
- auto *New = new VPInstruction(Opcode, Operands, getDebugLoc(), Name);
+ auto *New = new VPInstruction(Opcode, Operands, getDebugLoc());
New->transferFlags(*this);
return New;
}
@@ -2653,7 +2650,7 @@ class VPBasicBlock : public VPBlockBase {
/// Augment the existing recipes of a VPBasicBlock with an additional
/// \p Recipe as the last recipe.
- void appendRecipe(VPRecipeBase *Recipe) { insert(Recipe, end()); }
+ void appendRecipe(VPRecipeBase *Recipe, const Twine &Name = "");
/// The method which generates the output IR instructions that correspond to
/// this VPBasicBlock, thereby "executing" the VPlan.
@@ -2876,6 +2873,11 @@ class VPlan {
/// been modeled in VPlan directly.
DenseMap<const SCEV *, VPValue *> SCEVToExpansion;
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+ DenseMap<const VPValue *, std::string> VPValue2Name;
+ StringSet<> UsedNames;
+#endif
+
public:
/// Construct a VPlan with original preheader \p Preheader, trip count \p TC
/// and \p Entry to the plan. At the moment, \p Preheader and \p Entry need to
@@ -3016,6 +3018,42 @@ class VPlan {
LLVM_DUMP_METHOD void dump() const;
#endif
+ /// Set the name for \p V to \p Name if it is not empty. \p V must not have a
+ /// name assigned already.
+ void setName(const VPValue *V, const Twine &Name);
+
+ /// Remove the assigned name for \p V, if there is one.
+ void removeName(const VPValue *V) {
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+ if (VPValue2Name.contains(V)) {
+ UsedNames.erase(VPValue2Name[V]);
+ VPValue2Name.erase(V);
+ }
+#endif
+ }
+
+ /// Take the name for \p Src and move it to \p Dst. The name of \p Src will be
+ /// empty afterwards.
+ void takeName(const VPValue *Src, const VPValue *Dst) {
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+ std::string N = VPValue2Name.lookup(Src);
+ if (N.empty())
+ return;
+ VPValue2Name[Dst] = N;
+ VPValue2Name.erase(Src);
+#endif
+ }
+
+ /// Return the name assigned to \p V or an empty string if no name has been
+ /// assigned.
+ std::string getName(const VPValue *V) const {
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+ return VPValue2Name.lookup(V);
+#else
+ return "";
+#endif
+ }
+
/// Returns a range mapping the values the range \p Operands to their
/// corresponding VPValues.
iterator_range<mapped_iterator<Use *, std::function<VPValue *(Value *)>>>
diff --git a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
index 94456bf858d9c5..f5b34a740a21b0 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
@@ -297,7 +297,8 @@ void PlainCFGBuilder::createVPInstructionsForVPBB(VPBasicBlock *VPBB,
if (Br->isConditional()) {
VPValue *Cond = getOrCreateVPOperand(Br->getCondition());
VPBB->appendRecipe(
- new VPInstruction(VPInstruction::BranchOnCond, {Cond}));
+ new VPInstruction(VPInstruction::BranchOnCond, {Cond}),
+ Inst->getName());
}
// Skip the rest of the Instruction processing for Branch instructions.
@@ -310,7 +311,7 @@ void PlainCFGBuilder::createVPInstructionsForVPBB(VPBasicBlock *VPBB,
// an empty VPInstruction that we will fix once the whole plain CFG has
// been built.
NewVPV = new VPWidenPHIRecipe(Phi);
- VPBB->appendRecipe(cast<VPWidenPHIRecipe>(NewVPV));
+ VPBB->appendRecipe(cast<VPWidenPHIRecipe>(NewVPV), Inst->getName());
PhisToFix.push_back(Phi);
} else {
// Translate LLVM-IR operands into VPValue operands and set them in the
@@ -321,8 +322,8 @@ void PlainCFGBuilder::createVPInstructionsForVPBB(VPBasicBlock *VPBB,
// Build VPInstruction for any arbitrary Instruction without specific
// representation in VPlan.
- NewVPV = cast<VPInstruction>(
- VPIRBuilder.createNaryOp(Inst->getOpcode(), VPOperands, Inst));
+ NewVPV = cast<VPInstruction>(VPIRBuilder.createNaryOp(
+ Inst->getOpcode(), VPOperands, Inst, Inst->getName()));
}
IRDef2VPValue[Inst] = NewVPV;
diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
index 9ee0cb2bd61530..c24e0d72c222f7 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -226,6 +226,8 @@ void VPRecipeBase::removeFromParent() {
iplist<VPRecipeBase>::iterator VPRecipeBase::eraseFromParent() {
assert(getParent() && "Recipe not in any VPBasicBlock");
+ for (auto *V : definedValues())
+ getParent()->getPlan()->removeName(V);
return getParent()->getRecipeList().erase(getIterator());
}
@@ -255,20 +257,19 @@ FastMathFlags VPRecipeWithIRFlags::getFastMathFlags() const {
}
VPInstruction::VPInstruction(unsigned Opcode, CmpInst::Predicate Pred,
- VPValue *A, VPValue *B, DebugLoc DL,
- const Twine &Name)
+ VPValue *A, VPValue *B, DebugLoc DL)
: VPRecipeWithIRFlags(VPDef::VPInstructionSC, ArrayRef<VPValue *>({A, B}),
Pred, DL),
- Opcode(Opcode), Name(Name.str()) {
+ Opcode(Opcode) {
assert(Opcode == Instruction::ICmp &&
"only ICmp predicates supported at the moment");
}
VPInstruction::VPInstruction(unsigned Opcode,
std::initializer_list<VPValue *> Operands,
- FastMathFlags FMFs, DebugLoc DL, const Twine &Name)
+ FastMathFlags FMFs, DebugLoc DL)
: VPRecipeWithIRFlags(VPDef::VPInstructionSC, Operands, FMFs, DL),
- Opcode(Opcode), Name(Name.str()) {
+ Opcode(Opcode) {
// Make sure the VPInstruction is a floating-point operation.
assert(isFPMathOp() && "this op can't take fast-math flags");
}
@@ -277,6 +278,7 @@ Value *VPInstruction::generateInstruction(VPTransformState &State,
unsigned Part) {
IRBuilderBase &Builder = State.Builder;
Builder.SetCurrentDebugLocation(getDebugLoc());
+ std::string Name = getParent()->getPlan()->getName(this);
if (Instruction::isBinaryOp(getOpcode())) {
if (Part != 0 && vputils::onlyFirstPartUsed(this))
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
index 71f5285f90236b..bb1db219704439 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -88,9 +88,10 @@ void VPlanTransforms::VPInstructionsToVPRecipes(
}
NewRecipe->insertBefore(&Ingredient);
- if (NewRecipe->getNumDefinedValues() == 1)
+ if (NewRecipe->getNumDefinedValues() == 1) {
VPV->replaceAllUsesWith(NewRecipe->getVPSingleValue());
- else
+ NewRecipe->getVPSingleValue()->takeName(VPV);
+ } else
assert(NewRecipe->getNumDefinedValues() == 0 &&
"Only recpies with zero or one defined values expected");
Ingredient.eraseFromParent();
@@ -163,6 +164,8 @@ static bool sinkScalarOperands(VPlan &Plan) {
// TODO: add ".cloned" suffix to name of Clone's VPValue.
Clone->insertBefore(SinkCandidate);
+ if (!I->getName().empty())
+ Clone->setName(I->getName() + ".cloned");
SinkCandidate->replaceUsesWithIf(Clone, [SinkTo](VPUser &U, unsigned) {
return cast<VPRecipeBase>(&U)->getParent() != SinkTo;
});
@@ -318,6 +321,8 @@ static VPRegionBlock *createReplicateRegion(VPReplicateRecipe *PredRecipe,
PHIRecipe->setOperand(0, RecipeWithoutMask);
}
PredRecipe->eraseFromParent();
+ Plan.setName(RecipeWithoutMask,
+ RecipeWithoutMask->getUnderlyingValue()->getName());
auto *Exiting = new VPBasicBlock(Twine(RegionName) + ".continue", PHIRecipe);
VPRegionBlock *Region = new VPRegionBlock(Entry, Exiting, RegionName, true);
@@ -1177,10 +1182,11 @@ void VPl...
[truncated]
|
|
ping :) |
1 similar comment
|
ping :) |
|
It seems somewhat more natural to assign a name to a VPValue/single-VPValued-recipe upon construction (as done now for VPInstructions) rather than post-construction when it is inserted into a VPlan. The latter is required in order to ensure all names of a VPlan are unique, by having VPlan store all names of its VPValues in a set. Having a VPValue lose its name when taken out of its Block/VPlan, possibly temporarily, seems also less desirable. One alternative may be to utilize VPRecipeBuilder to create all (named) recipes, along with their preset VPlan/insertion-point. Another alternative may be to assign possibly duplicated "core" names to recipes upon construction, and check uniqueness later upon insertion into VPlan or even upon printing by VPSlotTracker, resolving duplication by adding the enumerating suffix (possibly removing the suffix upon removal from VPlan). If VPSlotTracker assigns suffixes, they would be assigned in program order, but only when dumping the entire VPlan. Another approach may be to associate a recipe with its VPlan upon construction, permanently and directly, rather than via its Block which is subject to insertion and removal. Another independent thought, raised by Gil, is to fill Opcode-dependent meaningful names such as "sum" for Add, "diff" for Sub, "prod" for Mul, etc. |
| @@ -446,6 +461,7 @@ class VPBasicBlock; | |||
| /// VPlan and allows querying the numbering for printing, similar to the | |||
| /// ModuleSlotTracker for IR values. | |||
| class VPSlotTracker { | |||
| const VPlan *Plan; | |||
There was a problem hiding this comment.
Does VPSlotTracker need Plan?
There was a problem hiding this comment.
Yes, it is needed to retrieve names for live-in VPValues that don't have underlying IR values
| @@ -8842,6 +8843,8 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) { | |||
| J++; | |||
| } | |||
| MemberR->eraseFromParent(); | |||
| if (!Member->getType()->isVoidTy()) | |||
There was a problem hiding this comment.
nit: could this be folded within the same if above?
There was a problem hiding this comment.
Yes, updated to take the name of MemberR
| if (UsedNames.contains(N)) | ||
| CurrName = N + ".1"; | ||
| unsigned Cnt = 2; | ||
| while (UsedNames.contains(CurrName)) { |
There was a problem hiding this comment.
This O(n^2) search is tolerable because n is expected to be very small? Can alternatively hold a map between used names and the number of times each name is used, instead of a set. The former will avoid reusing names that were removed.
There was a problem hiding this comment.
Yes, the number of clashes should be low.
The former will avoid reusing names that were removed.
The code at the moment intentionally allows re-using names for values that have been removed.
There was a problem hiding this comment.
It seems somewhat more natural to assign a name to a VPValue/single-VPValued-recipe upon construction (as done now for VPInstructions) rather than post-construction when it is inserted into a VPlan. The latter is required in order to ensure all names of a VPlan are unique, by having VPlan store all names of its VPValues in a set. Having a VPValue lose its name when taken out of its Block/VPlan, possibly temporarily, seems also less desirable.
One alternative may be to utilize VPRecipeBuilder to create all (named) recipes, along with their preset VPlan/insertion-point.
Another alternative may be to assign possibly duplicated "core" names to recipes upon construction, and check uniqueness later upon insertion into VPlan or even upon printing by VPSlotTracker, resolving duplication by adding the enumerating suffix (possibly removing the suffix upon removal from VPlan). If VPSlotTracker assigns suffixes, they would be assigned in program order, but only when dumping the entire VPlan.
Another approach may be to associate a recipe with its VPlan upon construction, permanently and directly, rather than via its Block which is subject to insertion and removal.
Another independent thought, raised by Gil, is to fill Opcode-dependent meaningful names such as "sum" for Add, "diff" for Sub, "prod" for Mul, etc.
I think overall it would be preferable to use either VPBuilder or VPRecipeBuilder for recipe construction + insertion. I updated some places to do so to remove the need to set names after the fact in most cases.
Also updated to remove passing the name to appendRecipe. Instead I updated ::insert to use the underlying value's name, if it has one automatically.
The current approach in the patch mirrors how names of Value are managed, but what makes this work not was nicely with VPlan is that most recipe's constructors don't have a reliable way to retrieve a VPlan before they are inserted (contrary to Value, where the context always can be retrieved via it's operands).
Alternatively we could add a std::string to VPValue to hold the name, and possibly indicate that the suffix may not be correct by adding ? as suffix unless a recipe has been inserted in a plan?
| @@ -446,6 +461,7 @@ class VPBasicBlock; | |||
| /// VPlan and allows querying the numbering for printing, similar to the | |||
| /// ModuleSlotTracker for IR values. | |||
| class VPSlotTracker { | |||
| const VPlan *Plan; | |||
There was a problem hiding this comment.
Yes, it is needed to retrieve names for live-in VPValues that don't have underlying IR values
| @@ -8842,6 +8843,8 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) { | |||
| J++; | |||
| } | |||
| MemberR->eraseFromParent(); | |||
| if (!Member->getType()->isVoidTy()) | |||
There was a problem hiding this comment.
Yes, updated to take the name of MemberR
| if (UsedNames.contains(N)) | ||
| CurrName = N + ".1"; | ||
| unsigned Cnt = 2; | ||
| while (UsedNames.contains(CurrName)) { |
There was a problem hiding this comment.
Yes, the number of clashes should be low.
The former will avoid reusing names that were removed.
The code at the moment intentionally allows re-using names for values that have been removed.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
On one hand, a name seems like a persistent property of a VPValue, to be initialized upon construction and retained regardless of being placed inside or being taken out of a VPlan, so perhaps an std::string within VPValue. Ensuring names are unique can either be done upon construction (in construction order, w/ or w/o recycling), possibly by introducing a VPlanContext to hold names of all VPlan entities, or upon printing (in program order), as already done by VPSlotTracker. OTOH,
So perhaps (1) dictate names for all recipes, derived from their class or opcode, and (2) inferring them uniquely when printed? |
Conflicts: llvm/lib/Transforms/Vectorize/VPlan.cpp
|
I completely reworked the patch to now only perform de-duplication in This should be sufficient to unblock VPlan2VPlan interleaving. WDYT? |
Looks good to me, with various minor comments.
Actually, it appears the (two listed VPInstruction) names do not even propagate to VPlan's debug printings, i.e., they are totally unused and should be dce'd. |
| void VPSlotTracker::assignSlot(const VPValue *V) { | ||
| if (V->getUnderlyingValue()) | ||
| void VPSlotTracker::assignSlotOrName(const VPValue *V) { | ||
| if (auto *UV = V->getUnderlyingValue()) { |
There was a problem hiding this comment.
nit: else case is simpler to go first. (Name also suggests Slot goes first ;)
| else | ||
| return (Twine("vp<%") + std::to_string(Slot) + ">").str(); |
There was a problem hiding this comment.
| else | |
| return (Twine("vp<%") + std::to_string(Slot) + ">").str(); | |
| return (Twine("vp<%") + std::to_string(Slot) + ">").str(); |
| } | ||
|
|
||
| void VPSlotTracker::deduplicateName(const VPValue *V, StringRef Name) { | ||
| assert(!Name.empty() && "Name cannot be be empty."); |
There was a problem hiding this comment.
| assert(!Name.empty() && "Name cannot be be empty."); | |
| assert(!Name.empty() && "Name cannot be empty."); |
| assignSlotOrName(Def); | ||
| } | ||
|
|
||
| void VPSlotTracker::deduplicateName(const VPValue *V, StringRef Name) { |
There was a problem hiding this comment.
"Deduplication" suggests folding multiple identical instances into one. "Versioning" as in SSA may more accurately refer to "Renaming" multiple identical instances so each becomes unique.
This does more than resolve duplication - it also records the (potentially renamed) name for later retrieval and subsequent name assignments.
There was a problem hiding this comment.
Thanks, updated to versionName
| assert(!Name.empty() && "Name cannot be be empty."); | ||
| std::string NewName = Name.str(); | ||
| const auto &[A, AssignedInserted] = AssignedNames.insert({V, NewName}); | ||
| if (!AssignedInserted || V->isLiveIn()) |
There was a problem hiding this comment.
Would be good to explain the logic here: first assign to V the BaseName provided, then if this BaseName is already used by C>0 other V's, augment the name assigned to V by ".{C++ - 1}".
Can V already have a name assigned in AssignedNames when calling deduplicateName()? I.e., assert !AssignedNames.contains(V) rather than check if insert fails (it should not)? OTOH, if insert may fail, to support renaming, assert that NewName is the same as the old one, or replace the latter with the former? If V is live-in - w/ or w/o underlying? - it should not be versioned?
There was a problem hiding this comment.
Updated and added comment. Returning on not inserting was from an earlier version that did on-demand numbering.
If V is live-in - w/ or w/o underlying? - it should not be versioned?
Yes, those will be assigned a slot, like other VPValues without underlying IR.
| class VPSlotTracker { | ||
| /// Keep track of de-duplicated names assigned to VPValues with underlying IR | ||
| /// values | ||
| DenseMap<const VPValue *, std::string> AssignedNames; |
There was a problem hiding this comment.
| DenseMap<const VPValue *, std::string> AssignedNames; | |
| DenseMap<const VPValue *, std::string> VPValue2Name; |
?
| /// values | ||
| DenseMap<const VPValue *, std::string> AssignedNames; | ||
| /// Keep track of the next number to use to deduplicate the base name. | ||
| StringMap<unsigned> NameUseCount; |
There was a problem hiding this comment.
| StringMap<unsigned> NameUseCount; | |
| StringMap<unsigned> BaseName2Count; |
?
| DenseMap<const VPValue *, std::string> AssignedNames; | ||
| /// Keep track of the next number to use to deduplicate the base name. | ||
| StringMap<unsigned> NameUseCount; | ||
|
|
||
| DenseMap<const VPValue *, unsigned> Slots; |
There was a problem hiding this comment.
While we're here, indepent:
| DenseMap<const VPValue *, unsigned> Slots; | |
| DenseMap<const VPValue *, unsigned> VPValue2Slot; |
?
There was a problem hiding this comment.
The slots map has been removed in the latest version.
| } | ||
| } | ||
|
|
||
| std::string VPSlotTracker::getName(const VPValue *V) const { |
There was a problem hiding this comment.
This does more than retrieve a precomputed name - it also takes part in its final building. Have assignSlotOrName record the final names, to keep its logic in one place and simplify retrieval?
There was a problem hiding this comment.
Updated to construct all names on initial assignment. We still need to handle the case where no names were assigned for cases where the slot tracker was constructed without VPlan, e.g. when printing a recipe via dump() that hasn't been inserted in a plan in a debugger.
For now, the code still retains the logic to use the underlying value name here, even if no name is assigned. Could be handled by updating VPSlotTracker's constructor to take a VPValue and assign a name/slot to
- the VPValue only, if it doesn't have a defining recipe
- the defining recipe only, if it hasn't been inserted in a VPlan
- the whole VPlan otherwise.
There was a problem hiding this comment.
Agreed! In these few cases, construct VPSlotTracker with a VPValue (and have it assign its name) instead of a VPlan.
There was a problem hiding this comment.
Best done as follow-up, as this will slightly change printing (for recipes with operands w/o underlying values, they would be numbered starting at 0 instead of using <badref>)?
There was a problem hiding this comment.
Can slot tracker assign a <badref> name in such cases?
Otherwise getName() would more accurately be called getOrCreateName().
But a follow-up would also be fine.
| @@ -466,6 +478,8 @@ class VPSlotTracker { | |||
| return -1; | |||
| return I->second; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Adde documentation, thanks!
Thanks, comments should be addressed, title + commit messages are updated |
fhahn
changed the title
| @@ -1373,32 +1362,84 @@ VPInterleavedAccessInfo::VPInterleavedAccessInfo(VPlan &Plan, | |||
| visitRegion(Plan.getVectorLoopRegion(), Old2New, IAI); | |||
| } | |||
|
|
|||
| void VPSlotTracker::assignSlot(const VPValue *V) { | |||
| if (V->getUnderlyingValue()) | |||
| void VPSlotTracker::assignSlotOrName(const VPValue *V) { | |||
There was a problem hiding this comment.
| void VPSlotTracker::assignSlotOrName(const VPValue *V) { | |
| void VPSlotTracker::assignName(const VPValue *V) { |
given that both names and slots are placed in VPValue2Name?
| auto *UV = V->getUnderlyingValue(); | ||
| if (!UV) { | ||
| assert(!VPValue2Name.contains(V) && "VPValue already has a slot!"); |
There was a problem hiding this comment.
| auto *UV = V->getUnderlyingValue(); | |
| if (!UV) { | |
| assert(!VPValue2Name.contains(V) && "VPValue already has a slot!"); | |
| assert(!VPValue2Name.contains(V) && "VPValue already has a name!"); | |
| auto *UV = V->getUnderlyingValue(); | |
| if (!UV) { |
| return; | ||
| assert(!Slots.contains(V) && "VPValue already has a slot!"); | ||
| Slots[V] = NextSlot++; | ||
| } |
There was a problem hiding this comment.
Worth adding a comment, e.g.,
// Use the name of the underlying Value, wrapped in "ir<>", and versioned.
| std::string Name; | ||
| raw_string_ostream S(Name); | ||
| UV->printAsOperand(S, false); | ||
| versionName(V, Name); |
There was a problem hiding this comment.
Worth inlining versionName()?
|
|
||
| // First assign the base name for V. | ||
| const auto &[A, AssignedInserted] = VPValue2Name.insert({V, BaseName}); | ||
| assert(AssignedInserted && "name assigned already?"); |
There was a problem hiding this comment.
This assert should be folded with the assert that VPValue2Name did not contain V, to handle both vp<> Slots and UV-based ir<> names.
| // First assign the base name for V. | ||
| const auto &[A, AssignedInserted] = VPValue2Name.insert({V, BaseName}); | ||
| assert(AssignedInserted && "name assigned already?"); | ||
| if (V->isLiveIn()) |
There was a problem hiding this comment.
Do we actually try to assign a name to the same live-in more than once?
There was a problem hiding this comment.
Some live-ins (integer/fp constants) will print to the same string due to the type being stripped. Clarified by only bailing out for those types + a comment
| std::string BaseName = (Twine("ir<") + Name + Twine(">")).str(); | ||
|
|
||
| // First assign the base name for V. | ||
| const auto &[A, AssignedInserted] = VPValue2Name.insert({V, BaseName}); |
There was a problem hiding this comment.
Better inline versionName(), so that both insertions into VPValue2Name appear in one place? Otherwise worth noting that versionName() also performs this insertion.
| } | ||
| } | ||
|
|
||
| std::string VPSlotTracker::getName(const VPValue *V) const { |
There was a problem hiding this comment.
Can slot tracker assign a <badref> name in such cases?
Otherwise getName() would more accurately be called getOrCreateName().
But a follow-up would also be fine.
| @@ -1373,32 +1362,84 @@ VPInterleavedAccessInfo::VPInterleavedAccessInfo(VPlan &Plan, | |||
| visitRegion(Plan.getVectorLoopRegion(), Old2New, IAI); | |||
| } | |||
|
|
|||
| void VPSlotTracker::assignSlot(const VPValue *V) { | |||
| if (V->getUnderlyingValue()) | |||
| void VPSlotTracker::assignSlotOrName(const VPValue *V) { | |||
| auto *UV = V->getUnderlyingValue(); | ||
| if (!UV) { | ||
| assert(!VPValue2Name.contains(V) && "VPValue already has a slot!"); |
| return; | ||
| assert(!Slots.contains(V) && "VPValue already has a slot!"); | ||
| Slots[V] = NextSlot++; | ||
| } |
| std::string Name; | ||
| raw_string_ostream S(Name); | ||
| UV->printAsOperand(S, false); | ||
| versionName(V, Name); |
| std::string BaseName = (Twine("ir<") + Name + Twine(">")).str(); | ||
|
|
||
| // First assign the base name for V. | ||
| const auto &[A, AssignedInserted] = VPValue2Name.insert({V, BaseName}); |
|
|
||
| // First assign the base name for V. | ||
| const auto &[A, AssignedInserted] = VPValue2Name.insert({V, BaseName}); | ||
| assert(AssignedInserted && "name assigned already?"); |
| // First assign the base name for V. | ||
| const auto &[A, AssignedInserted] = VPValue2Name.insert({V, BaseName}); | ||
| assert(AssignedInserted && "name assigned already?"); | ||
| if (V->isLiveIn()) |
There was a problem hiding this comment.
Some live-ins (integer/fp constants) will print to the same string due to the type being stripped. Clarified by only bailing out for those types + a comment
| } | ||
| } | ||
|
|
||
| std::string VPSlotTracker::getName(const VPValue *V) const { |
| /// This class can be used to assign names to VPValues. For VPValues without | ||
| /// underlying value, assign consecutive numbers and use those as names (wrapped | ||
| /// in vp<>). Otherwise, use the name from the underlying value (wrapped in | ||
| /// ir<>), apending a .V version number if there are multiple uses of the same |
There was a problem hiding this comment.
| /// ir<>), apending a .V version number if there are multiple uses of the same | |
| /// ir<>), appending a .V version number if there are multiple uses of the same |
| /// underlying value, assign consecutive numbers and use those as names (wrapped | ||
| /// in vp<>). Otherwise, use the name from the underlying value (wrapped in | ||
| /// ir<>), apending a .V version number if there are multiple uses of the same | ||
| /// name. Allows querying names for VPValues for printing, similar to the |
There was a problem hiding this comment.
| /// name. Allows querying names for VPValues for printing, similar to the | |
| /// name. Allows querying names for VPValues for printing, similar to the |
| /// ModuleSlotTracker for IR values. | ||
| class VPSlotTracker { | ||
| DenseMap<const VPValue *, unsigned> Slots; | ||
| /// Keep track of versioned names assigned to VPValues with underlying IR | ||
| /// values |
There was a problem hiding this comment.
| /// values | |
| /// values. |
| /// Keep track of versioned names assigned to VPValues with underlying IR | ||
| /// values | ||
| DenseMap<const VPValue *, std::string> VPValue2Name; | ||
| /// Keep track of the next number to use to deduplicate the base name. |
There was a problem hiding this comment.
| /// Keep track of the next number to use to deduplicate the base name. | |
| /// Keep track of the next number to use to version the base name. |
| @@ -986,8 +986,8 @@ define void @sinking_requires_duplication(ptr %addr) { | |||
| ; CHECK-NEXT: Successor(s): pred.store.if, pred.store.continue | |||
| ; CHECK-EMPTY: | |||
| ; CHECK-NEXT: pred.store.if: | |||
| ; CHECK-NEXT: REPLICATE ir<%gep> = getelementptr ir<%addr>, vp<[[STEPS]]> | |||
| ; CHECK-NEXT: REPLICATE store ir<1.000000e+01>, ir<%gep> | |||
| ; CHECK-NEXT: REPLICATE ir<%gep>.1 = getelementptr ir<%addr>, vp<[[STEPS]]> | |||
There was a problem hiding this comment.
Thanks, this naming should clarify the relation to the original IR. It hopefully also clarifies the relation to the generated IR?
Add extra tests for printing recipes not inserted in a VPlan yet, e.g. when using a debugger. Guard against regressions in changes to printing, i.e. #81411.
Add extra tests for printing recipes not inserted in a VPlan yet, e.g. when using a debugger. Guard against regressions in changes to printing, i.e. llvm#81411.
This patch restructures the way names for printing VPValues are handled. It moves the logic to generate names for printing to VPSlotTracker. VPSlotTracker will now version names of the same underlying value if it is used by multiple VPValues, by adding a .V suffix to the name. This fixes cases where at the moment the same name is printed for different VPValues. PR: llvm#81411
Add extra tests for printing recipes not inserted in a VPlan yet, e.g. when using a debugger. Guard against regressions in changes to printing, i.e. llvm#81411.
This patch restructures the way names for printing VPValues are handled. It moves the logic to generate names for printing to VPSlotTracker. VPSlotTracker will now version names of the same underlying value if it is used by multiple VPValues, by adding a .V suffix to the name. This fixes cases where at the moment the same name is printed for different VPValues. PR: llvm#81411
This patch restructures the way names for printing VPValues are handled.
It moves the logic to generate names for printing to VPSlotTracker.
VPSlotTracker will now version names of the same underlying value if it
is used by multiple VPValues, by adding a .V suffix to the name.
This fixes cases where at the moment the same name is printed for
different VPValues.