[llvm] 8bbe0d0 - [VPlan] Verify dominance for incoming values of phi-like recipes. (#124838)
via llvm-commits
llvm-commits at lists.llvm.org
Thu May 15 04:20:58 PDT 2025
Author: Florian Hahn
Date: 2025-05-15T12:20:54+01:00
New Revision: 8bbe0d050ae49062835a8a0729b3219ba1f6362b
URL: https://github.com/llvm/llvm-project/commit/8bbe0d050ae49062835a8a0729b3219ba1f6362b
DIFF: https://github.com/llvm/llvm-project/commit/8bbe0d050ae49062835a8a0729b3219ba1f6362b.diff
LOG: [VPlan] Verify dominance for incoming values of phi-like recipes. (#124838)
Update the verifier to verify dominance for incoming values for phi-like
recipes. The defining recipe must dominate the incoming block for the
incoming value.
Builds on top of https://github.com/llvm/llvm-project/pull/138472 to
retrieve incoming values & corresponding blocks for phi-like recipes.
PR: https://github.com/llvm/llvm-project/pull/124838
Added:
Modified:
llvm/lib/Transforms/Vectorize/VPlan.h
llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
llvm/unittests/Transforms/Vectorize/VPlanVerifierTest.cpp
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 2c4cac7655ec9..5fd7a369bf735 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -1135,7 +1135,9 @@ class VPPhiAccessors {
const VPBasicBlock *getIncomingBlock(unsigned Idx) const;
/// Returns the number of incoming values, also number of incoming blocks.
- unsigned getNumIncoming() const { return getAsRecipe()->getNumOperands(); }
+ virtual unsigned getNumIncoming() const {
+ return getAsRecipe()->getNumOperands();
+ }
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
/// Print the recipe.
@@ -1234,7 +1236,7 @@ class VPIRInstruction : public VPRecipeBase {
/// cast/dyn_cast/isa and execute() implementation. A single VPValue operand is
/// allowed, and it is used to add a new incoming value for the single
/// predecessor VPBB.
-struct VPIRPhi : public VPIRInstruction {
+struct VPIRPhi : public VPIRInstruction, public VPPhiAccessors {
VPIRPhi(PHINode &PN) : VPIRInstruction(PN) {}
static inline bool classof(const VPRecipeBase *U) {
@@ -1251,6 +1253,9 @@ struct VPIRPhi : public VPIRInstruction {
void print(raw_ostream &O, const Twine &Indent,
VPSlotTracker &SlotTracker) const override;
#endif
+
+protected:
+ const VPRecipeBase *getAsRecipe() const override { return this; }
};
/// Helper to manage IR metadata for recipes. It filters out metadata that
@@ -1785,13 +1790,15 @@ class VPVectorPointerRecipe : public VPRecipeWithIRFlags,
/// * VPWidenPointerInductionRecipe: Generate vector and scalar values for a
/// pointer induction. Produces either a vector PHI per-part or scalar values
/// per-lane based on the canonical induction.
-class VPHeaderPHIRecipe : public VPSingleDefRecipe {
+class VPHeaderPHIRecipe : public VPSingleDefRecipe, public VPPhiAccessors {
protected:
VPHeaderPHIRecipe(unsigned char VPDefID, Instruction *UnderlyingInstr,
VPValue *Start, DebugLoc DL = {})
: VPSingleDefRecipe(VPDefID, ArrayRef<VPValue *>({Start}), UnderlyingInstr, DL) {
}
+ const VPRecipeBase *getAsRecipe() const override { return this; }
+
public:
~VPHeaderPHIRecipe() override = default;
@@ -1980,6 +1987,11 @@ class VPWidenIntOrFpInductionRecipe : public VPWidenInductionRecipe {
return isUnrolled() ? getOperand(getNumOperands() - 2) : nullptr;
}
+ /// Returns the number of incoming values, also number of incoming blocks.
+ /// Note that at the moment, VPWidenIntOrFpInductionRecipes only have a single
+ /// incoming value, its start value.
+ unsigned getNumIncoming() const override { return 1; }
+
/// Returns the first defined value as TruncInst, if it is one or nullptr
/// otherwise.
TruncInst *getTruncInst() { return Trunc; }
@@ -3283,6 +3295,46 @@ class VPScalarIVStepsRecipe : public VPRecipeWithIRFlags,
}
};
+/// Casting from VPRecipeBase -> VPPhiAccessors is supported for all recipe
+/// types implementing VPPhiAccessors. Used by isa<> & co.
+template <> struct CastIsPossible<VPPhiAccessors, const VPRecipeBase *> {
+ static inline bool isPossible(const VPRecipeBase *f) {
+ // TODO: include VPPredInstPHIRecipe too, once it implements VPPhiAccessors.
+ return isa<VPIRPhi, VPHeaderPHIRecipe, VPWidenPHIRecipe, VPPhi>(f);
+ }
+};
+/// Support casting from VPRecipeBase -> VPPhiAccessors, by down-casting to the
+/// recipe types implementing VPPhiAccessors. Used by cast<>, dyn_cast<> & co.
+template <>
+struct CastInfo<VPPhiAccessors, const VPRecipeBase *>
+ : public CastIsPossible<VPPhiAccessors, const VPRecipeBase *> {
+
+ using Self = CastInfo<VPPhiAccessors, const VPRecipeBase *>;
+
+ /// doCast is used by cast<>.
+ static inline VPPhiAccessors *doCast(const VPRecipeBase *R) {
+ return const_cast<VPPhiAccessors *>([R]() -> const VPPhiAccessors * {
+ switch (R->getVPDefID()) {
+ case VPDef::VPInstructionSC:
+ return cast<VPPhi>(R);
+ case VPDef::VPIRInstructionSC:
+ return cast<VPIRPhi>(R);
+ case VPDef::VPWidenPHISC:
+ return cast<VPWidenPHIRecipe>(R);
+ default:
+ return cast<VPHeaderPHIRecipe>(R);
+ }
+ }());
+ }
+
+ /// doCastIfPossible is used by dyn_cast<>.
+ static inline VPPhiAccessors *doCastIfPossible(const VPRecipeBase *f) {
+ if (!Self::isPossible(f))
+ return nullptr;
+ return doCast(f);
+ }
+};
+
/// VPBasicBlock serves as the leaf of the Hierarchical Control-Flow Graph. It
/// holds a sequence of zero or more VPRecipe's each representing a sequence of
/// output IR instructions. All PHI-like recipes must come before any non-PHI recipes.
diff --git a/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp b/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
index b8205545a4f5e..1e7e039a18d56 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
@@ -192,8 +192,7 @@ bool VPlanVerifier::verifyVPBasicBlock(const VPBasicBlock *VPBB) {
if (!verifyPhiRecipes(VPBB))
return false;
- // Verify that defs in VPBB dominate all their uses. The current
- // implementation is still incomplete.
+ // Verify that defs in VPBB dominate all their uses.
DenseMap<const VPRecipeBase *, unsigned> RecipeNumbering;
unsigned Cnt = 0;
for (const VPRecipeBase &R : *VPBB)
@@ -220,12 +219,31 @@ bool VPlanVerifier::verifyVPBasicBlock(const VPBasicBlock *VPBB) {
for (const VPUser *U : V->users()) {
auto *UI = cast<VPRecipeBase>(U);
- // TODO: check dominance of incoming values for phis properly.
- if (!UI ||
- isa<VPHeaderPHIRecipe, VPWidenPHIRecipe, VPPredInstPHIRecipe,
- VPIRPhi>(UI) ||
- (isa<VPInstruction>(UI) &&
- cast<VPInstruction>(UI)->getOpcode() == Instruction::PHI))
+ if (auto *Phi = dyn_cast<VPPhiAccessors>(UI)) {
+ for (unsigned Idx = 0; Idx != Phi->getNumIncoming(); ++Idx) {
+ VPValue *IncomingVPV = Phi->getIncomingValue(Idx);
+ if (IncomingVPV != V)
+ continue;
+
+ const VPBasicBlock *IncomingVPBB = Phi->getIncomingBlock(Idx);
+ if (VPDT.dominates(VPBB, IncomingVPBB))
+ continue;
+
+ errs() << "Incoming def at index " << Idx
+ << " does not dominate incoming block!\n";
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+ VPSlotTracker Tracker(VPBB->getPlan());
+ IncomingVPV->getDefiningRecipe()->print(errs(), " ", Tracker);
+ errs() << "\n does not dominate " << IncomingVPBB->getName()
+ << " for\n";
+ UI->print(errs(), " ", Tracker);
+#endif
+ return false;
+ }
+ continue;
+ }
+ // TODO: Also verify VPPredInstPHIRecipe.
+ if (isa<VPPredInstPHIRecipe>(UI))
continue;
// If the user is in the same block, check it comes after R in the
diff --git a/llvm/unittests/Transforms/Vectorize/VPlanVerifierTest.cpp b/llvm/unittests/Transforms/Vectorize/VPlanVerifierTest.cpp
index 84b7e33146811..9e0196c44f441 100644
--- a/llvm/unittests/Transforms/Vectorize/VPlanVerifierTest.cpp
+++ b/llvm/unittests/Transforms/Vectorize/VPlanVerifierTest.cpp
@@ -143,6 +143,43 @@ TEST_F(VPVerifierTest, VPBlendUseBeforeDefDifferentBB) {
delete Phi;
}
+TEST_F(VPVerifierTest, VPPhiIncomingValueDoesntDominateIncomingBlock) {
+ VPlan &Plan = getPlan();
+ IntegerType *Int32 = IntegerType::get(C, 32);
+ VPValue *Zero = Plan.getOrAddLiveIn(ConstantInt::get(Int32, 0));
+
+ VPBasicBlock *VPBB1 = Plan.getEntry();
+ VPBasicBlock *VPBB2 = Plan.createVPBasicBlock("");
+ VPBasicBlock *VPBB3 = Plan.createVPBasicBlock("");
+
+ VPInstruction *DefI = new VPInstruction(Instruction::Add, {Zero});
+ VPPhi *Phi = new VPPhi({DefI}, {});
+ VPBB2->appendRecipe(Phi);
+ VPBB2->appendRecipe(DefI);
+ auto *CanIV = new VPCanonicalIVPHIRecipe(Zero, {});
+ VPBB3->appendRecipe(CanIV);
+
+ VPRegionBlock *R1 = Plan.createVPRegionBlock(VPBB3, VPBB3, "R1");
+ VPBlockUtils::connectBlocks(VPBB1, VPBB2);
+ VPBlockUtils::connectBlocks(VPBB2, R1);
+#if GTEST_HAS_STREAM_REDIRECTION
+ ::testing::internal::CaptureStderr();
+#endif
+ EXPECT_FALSE(verifyVPlanIsValid(Plan));
+#if GTEST_HAS_STREAM_REDIRECTION
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+ EXPECT_STREQ("Incoming def at index 0 does not dominate incoming block!\n"
+ " EMIT vp<%2> = add ir<0>\n"
+ " does not dominate preheader for\n"
+ " EMIT vp<%1> = phi [ vp<%2>, preheader ]",
+ ::testing::internal::GetCapturedStderr().c_str());
+#else
+ EXPECT_STREQ("Use before def!\n",
+ ::testing::internal::GetCapturedStderr().c_str());
+#endif
+#endif
+}
+
TEST_F(VPVerifierTest, DuplicateSuccessorsOutsideRegion) {
VPlan &Plan = getPlan();
VPValue *Zero = Plan.getOrAddLiveIn(ConstantInt::get(Type::getInt32Ty(C), 0));
More information about the llvm-commits
mailing list