[VPlan] Assert vplan-verify-each result and fix LastActiveLane verification#182254
Merged
[VPlan] Assert vplan-verify-each result and fix LastActiveLane verification#182254
Conversation
Currently if -vplan-verify-each is enabled and a pass fails the verifier, it will output the failure to stderr but will still finish with a zero exit code. This adds an assert that the verification fails so that e.g. lit will pick up verifier failures in the in-tree tests with an EXPENSIVE_CHECKS build. However currently the verifier fails in several tests, so this also includes several fixes: 1. Remove the EVL verifier checks. The EVL is used in quite a few more places than when the verification was originally added, and nowadays it gets used in a variety of VPInstructions, all of which need to be handled by the verifier. There are a few passes that fail today because we haven't updated the EVL verification. Now that the EVL transform has been split up into a variable stepping transform and an optimization pass it's generally less fragile, so there is less value in this verification. 2. Extend the LastActiveLane verification to handle more prefix masks. All of the prefix masks that the verifier encounters are of the form `icmp ult/ule monotonically-increasing-sequence, uniform`, which always generate a prefix mask. 3. Allow multiple VPActiveLaneMaskPHIRecipe recipes after unrolling. This also allows us to remove the verifyLate argument Tested that llvm-test-suite + SPEC CPU 2017 now pass with -vplan-verify-each enabled for RISC-V.
Member
|
@llvm/pr-subscribers-vectorizers Author: Luke Lau (lukel97) ChangesCurrently if -vplan-verify-each is enabled and a pass fails the verifier, it will output the failure to stderr but will still finish with a zero exit code. This adds an assert that the verification fails so that e.g. lit will pick up verifier failures in the in-tree tests with an EXPENSIVE_CHECKS build. However currently the verifier fails in several tests, so this also includes several fixes:
Tested that llvm-test-suite + SPEC CPU 2017 now pass with -vplan-verify-each enabled for RISC-V. Full diff: https://github.com/llvm/llvm-project/pull/182254.diff 4 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 01433fe4c4ba7..68bdb9f4ba23f 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -364,7 +364,7 @@ cl::opt<bool>
cl::init(false),
#endif
cl::Hidden,
- cl::desc("Verfiy VPlans after VPlan transforms."));
+ cl::desc("Verify VPlans after VPlan transforms."));
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
cl::opt<bool> llvm::VPlanPrintAfterAll(
@@ -7512,8 +7512,7 @@ DenseMap<const SCEV *, Value *> LoopVectorizationPlanner::executePlan(
State.CFG.PrevBB->getSingleSuccessor(), &BestVPlan);
VPlanTransforms::removeDeadRecipes(BestVPlan);
- assert(verifyVPlanIsValid(BestVPlan, true /*VerifyLate*/) &&
- "final VPlan is invalid");
+ assert(verifyVPlanIsValid(BestVPlan) && "final VPlan is invalid");
// After vectorization, the exit blocks of the original loop will have
// additional predecessors. Invalidate SCEVs for the exit phis in case SE
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.h b/llvm/lib/Transforms/Vectorize/VPlanTransforms.h
index f2dfc166cecc9..e35872bbdff37 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.h
@@ -72,8 +72,10 @@ struct VPlanTransforms {
dbgs() << Plan << '\n';
}
#endif
- if (VerifyEachVPlan && EnableVerify)
- verifyVPlanIsValid(Plan);
+ if (VerifyEachVPlan && EnableVerify) {
+ [[maybe_unused]] bool IsValid = verifyVPlanIsValid(Plan);
+ assert(IsValid && "VPlan is invalid");
+ }
}};
return std::forward<PassTy>(Pass)(Plan, std::forward<ArgsTy>(Args)...);
diff --git a/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp b/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
index e4b334c3eba49..2db7287d494f6 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
@@ -31,7 +31,6 @@ namespace {
class VPlanVerifier {
const VPDominatorTree &VPDT;
VPTypeAnalysis &TypeInfo;
- bool VerifyLate;
SmallPtrSet<BasicBlock *, 8> WrappedIRBBs;
@@ -40,11 +39,6 @@ class VPlanVerifier {
// VPHeaderPHIRecipes.
bool verifyPhiRecipes(const VPBasicBlock *VPBB);
- /// Verify that \p EVL is used correctly. The user must be either in
- /// EVL-based recipes as a last operand or VPInstruction::Add which is
- /// incoming value into EVL's recipe.
- bool verifyEVLRecipe(const VPInstruction &EVL) const;
-
/// Verify that \p LastActiveLane's operand is guaranteed to be a prefix-mask.
bool verifyLastActiveLaneRecipe(const VPInstruction &LastActiveLane) const;
@@ -67,9 +61,8 @@ class VPlanVerifier {
bool verifyRegionRec(const VPRegionBlock *Region);
public:
- VPlanVerifier(VPDominatorTree &VPDT, VPTypeAnalysis &TypeInfo,
- bool VerifyLate)
- : VPDT(VPDT), TypeInfo(TypeInfo), VerifyLate(VerifyLate) {}
+ VPlanVerifier(VPDominatorTree &VPDT, VPTypeAnalysis &TypeInfo)
+ : VPDT(VPDT), TypeInfo(TypeInfo) {}
bool verify(const VPlan &Plan);
};
@@ -124,7 +117,7 @@ bool VPlanVerifier::verifyPhiRecipes(const VPBasicBlock *VPBB) {
RecipeI++;
}
- if (!VerifyLate && NumActiveLaneMaskPhiRecipes > 1) {
+ if (!VPBB->getPlan()->isUnrolled() && NumActiveLaneMaskPhiRecipes > 1) {
errs() << "There should be no more than one VPActiveLaneMaskPHIRecipe";
return false;
}
@@ -146,106 +139,19 @@ bool VPlanVerifier::verifyPhiRecipes(const VPBasicBlock *VPBB) {
return true;
}
-bool VPlanVerifier::verifyEVLRecipe(const VPInstruction &EVL) const {
- if (EVL.getOpcode() != VPInstruction::ExplicitVectorLength) {
- errs() << "verifyEVLRecipe should only be called on "
- "VPInstruction::ExplicitVectorLength\n";
- return false;
- }
- auto VerifyEVLUse = [&](const VPRecipeBase &R,
- const unsigned ExpectedIdx) -> bool {
- SmallVector<const VPValue *> Ops(R.operands());
- unsigned UseCount = count(Ops, &EVL);
- if (UseCount != 1 || Ops[ExpectedIdx] != &EVL) {
- errs() << "EVL is used as non-last operand in EVL-based recipe\n";
- return false;
- }
+static bool isKnownMonotonic(VPValue *V) {
+ VPValue *X, *Y;
+ if (match(V, m_Add(m_VPValue(X), m_VPValue(Y))))
+ return isKnownMonotonic(X) && isKnownMonotonic(Y);
+ if (match(V, m_StepVector()))
return true;
- };
- auto VerifyEVLUseInVecEndPtr = [&EVL](auto &VEPRs) {
- if (all_of(VEPRs, [&EVL](VPUser *U) {
- auto *VEPR = cast<VPVectorEndPointerRecipe>(U);
- return match(VEPR->getOffset(),
- m_c_Mul(m_SpecificSInt(VEPR->getStride()),
- m_Sub(m_Specific(&EVL), m_One())));
- }))
- return true;
- errs() << "Expected VectorEndPointer with EVL operand\n";
- return false;
- };
- return all_of(EVL.users(), [&](VPUser *U) {
- return TypeSwitch<const VPUser *, bool>(U)
- .Case([&](const VPWidenIntrinsicRecipe *S) {
- return VerifyEVLUse(*S, S->getNumOperands() - 1);
- })
- .Case<VPWidenStoreEVLRecipe, VPReductionEVLRecipe,
- VPWidenIntOrFpInductionRecipe, VPWidenPointerInductionRecipe>(
- [&](const VPRecipeBase *S) { return VerifyEVLUse(*S, 2); })
- .Case([&](const VPScalarIVStepsRecipe *R) {
- if (R->getNumOperands() != 3) {
- errs() << "Unrolling with EVL tail folding not yet supported\n";
- return false;
- }
- return VerifyEVLUse(*R, 2);
- })
- .Case<VPWidenLoadEVLRecipe, VPVectorEndPointerRecipe,
- VPInterleaveEVLRecipe>(
- [&](const VPRecipeBase *R) { return VerifyEVLUse(*R, 1); })
- .Case(
- [&](const VPInstructionWithType *S) { return VerifyEVLUse(*S, 0); })
- .Case([&](const VPInstruction *I) {
- if (I->getOpcode() == Instruction::PHI ||
- I->getOpcode() == Instruction::ICmp)
- return VerifyEVLUse(*I, 1);
- if (I->getOpcode() == Instruction::Sub) {
- // If Sub has a single user that's a SingleDefRecipe (which is
- // expected to be a Mul), filter its users, in turn, to get
- // VectorEndPointerRecipes, and verify that all the offsets match
- // (EVL - 1) * Stride.
- if (auto *Def = dyn_cast_if_present<VPSingleDefRecipe>(
- I->getSingleUser())) {
- auto VEPRs = make_filter_range(Def->users(),
- IsaPred<VPVectorEndPointerRecipe>);
- if (!VEPRs.empty())
- return VerifyEVLUseInVecEndPtr(VEPRs);
- }
- return VerifyEVLUse(*I, 1);
- }
- switch (I->getOpcode()) {
- case Instruction::Add:
- break;
- case Instruction::UIToFP:
- case Instruction::Trunc:
- case Instruction::ZExt:
- case Instruction::Mul:
- case Instruction::Shl:
- case Instruction::FMul:
- case VPInstruction::Broadcast:
- case VPInstruction::PtrAdd:
- // Opcodes above can only use EVL after wide inductions have been
- // expanded.
- if (!VerifyLate) {
- errs() << "EVL used by unexpected VPInstruction\n";
- return false;
- }
- break;
- default:
- errs() << "EVL used by unexpected VPInstruction\n";
- return false;
- }
- if (!VerifyLate &&
- !isa<VPCurrentIterationPHIRecipe>(*I->users().begin())) {
- errs() << "Result of VPInstruction::Add with EVL operand is "
- "not used by VPCurrentIterationPHIRecipe\n";
- return false;
- }
- return true;
- })
- .Default([&](const VPUser *U) {
- errs() << "EVL has unexpected user\n";
- return false;
- });
- });
+ if (auto *WidenIV = dyn_cast<VPWidenIntOrFpInductionRecipe>(V))
+ return match(WidenIV->getStepValue(), m_One());
+ if (auto *WidenIV = dyn_cast<VPScalarIVStepsRecipe>(V))
+ return match(WidenIV->getStepValue(), m_One());
+ if (isa<VPWidenCanonicalIVRecipe>(V))
+ return true;
+ return vputils::isUniformAcrossVFsAndUFs(V);
}
bool VPlanVerifier::verifyLastActiveLaneRecipe(
@@ -259,18 +165,19 @@ bool VPlanVerifier::verifyLastActiveLaneRecipe(
}
const VPlan &Plan = *LastActiveLane.getParent()->getPlan();
- // All operands must be prefix-mask. Currently we check for header masks or
- // EVL-derived masks, as those are currently the only operands in practice,
- // but this may need updating in the future.
+ // All operands must be prefix-mask. This means an icmp ult/ule LHS, RHS where
+ // the LHS is monotonically increasing and RHS is uniform.
for (VPValue *Op : LastActiveLane.operands()) {
if (vputils::isHeaderMask(Op, Plan))
continue;
- // Masks derived from EVL are also fine.
- auto BroadcastOrEVL =
- m_CombineOr(m_Broadcast(m_EVL(m_VPValue())), m_EVL(m_VPValue()));
- if (match(Op, m_CombineOr(m_ICmp(m_StepVector(), BroadcastOrEVL),
- m_ICmp(BroadcastOrEVL, m_StepVector()))))
+ CmpPredicate Pred;
+ VPValue *LHS, *RHS;
+ if (match(Op, m_ICmp(Pred, m_VPValue(LHS), m_VPValue(RHS))) &&
+ (Pred == CmpInst::ICMP_ULE || Pred == CmpInst::ICMP_ULT) &&
+ isKnownMonotonic(LHS) &&
+ (vputils::isUniformAcrossVFsAndUFs(RHS) ||
+ vputils::isSingleScalar(RHS)))
continue;
errs() << "LastActiveLane operand ";
@@ -372,12 +279,6 @@ bool VPlanVerifier::verifyVPBasicBlock(const VPBasicBlock *VPBB) {
}
if (const auto *VPI = dyn_cast<VPInstruction>(&R)) {
switch (VPI->getOpcode()) {
- case VPInstruction::ExplicitVectorLength:
- if (!verifyEVLRecipe(*VPI)) {
- errs() << "EVL VPValue is not used correctly\n";
- return false;
- }
- break;
case VPInstruction::LastActiveLane:
if (!verifyLastActiveLaneRecipe(*VPI))
return false;
@@ -569,9 +470,9 @@ bool VPlanVerifier::verify(const VPlan &Plan) {
return true;
}
-bool llvm::verifyVPlanIsValid(const VPlan &Plan, bool VerifyLate) {
+bool llvm::verifyVPlanIsValid(const VPlan &Plan) {
VPDominatorTree VPDT(const_cast<VPlan &>(Plan));
VPTypeAnalysis TypeInfo(Plan);
- VPlanVerifier Verifier(VPDT, TypeInfo, VerifyLate);
+ VPlanVerifier Verifier(VPDT, TypeInfo);
return Verifier.verify(Plan);
}
diff --git a/llvm/lib/Transforms/Vectorize/VPlanVerifier.h b/llvm/lib/Transforms/Vectorize/VPlanVerifier.h
index ccf79e8e5c985..642b25b0635c0 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanVerifier.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanVerifier.h
@@ -29,16 +29,14 @@
namespace llvm {
class VPlan;
-/// Verify invariants for general VPlans. If \p VerifyLate is passed, skip some
-/// checks that are not applicable at later stages of the transform pipeline.
+/// Verify invariants for general VPlans.
/// Currently it checks the following:
/// 1. Region/Block verification: Check the Region/Block verification
/// invariants for every region in the H-CFG.
/// 2. all phi-like recipes must be at the beginning of a block, with no other
/// recipes in between. Note that currently there is still an exception for
/// VPBlendRecipes.
-LLVM_ABI_FOR_TEST bool verifyVPlanIsValid(const VPlan &Plan,
- bool VerifyLate = false);
+LLVM_ABI_FOR_TEST bool verifyVPlanIsValid(const VPlan &Plan);
} // namespace llvm
|
Member
|
@llvm/pr-subscribers-llvm-transforms Author: Luke Lau (lukel97) ChangesCurrently if -vplan-verify-each is enabled and a pass fails the verifier, it will output the failure to stderr but will still finish with a zero exit code. This adds an assert that the verification fails so that e.g. lit will pick up verifier failures in the in-tree tests with an EXPENSIVE_CHECKS build. However currently the verifier fails in several tests, so this also includes several fixes:
Tested that llvm-test-suite + SPEC CPU 2017 now pass with -vplan-verify-each enabled for RISC-V. Full diff: https://github.com/llvm/llvm-project/pull/182254.diff 4 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 01433fe4c4ba7..68bdb9f4ba23f 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -364,7 +364,7 @@ cl::opt<bool>
cl::init(false),
#endif
cl::Hidden,
- cl::desc("Verfiy VPlans after VPlan transforms."));
+ cl::desc("Verify VPlans after VPlan transforms."));
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
cl::opt<bool> llvm::VPlanPrintAfterAll(
@@ -7512,8 +7512,7 @@ DenseMap<const SCEV *, Value *> LoopVectorizationPlanner::executePlan(
State.CFG.PrevBB->getSingleSuccessor(), &BestVPlan);
VPlanTransforms::removeDeadRecipes(BestVPlan);
- assert(verifyVPlanIsValid(BestVPlan, true /*VerifyLate*/) &&
- "final VPlan is invalid");
+ assert(verifyVPlanIsValid(BestVPlan) && "final VPlan is invalid");
// After vectorization, the exit blocks of the original loop will have
// additional predecessors. Invalidate SCEVs for the exit phis in case SE
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.h b/llvm/lib/Transforms/Vectorize/VPlanTransforms.h
index f2dfc166cecc9..e35872bbdff37 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.h
@@ -72,8 +72,10 @@ struct VPlanTransforms {
dbgs() << Plan << '\n';
}
#endif
- if (VerifyEachVPlan && EnableVerify)
- verifyVPlanIsValid(Plan);
+ if (VerifyEachVPlan && EnableVerify) {
+ [[maybe_unused]] bool IsValid = verifyVPlanIsValid(Plan);
+ assert(IsValid && "VPlan is invalid");
+ }
}};
return std::forward<PassTy>(Pass)(Plan, std::forward<ArgsTy>(Args)...);
diff --git a/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp b/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
index e4b334c3eba49..2db7287d494f6 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
@@ -31,7 +31,6 @@ namespace {
class VPlanVerifier {
const VPDominatorTree &VPDT;
VPTypeAnalysis &TypeInfo;
- bool VerifyLate;
SmallPtrSet<BasicBlock *, 8> WrappedIRBBs;
@@ -40,11 +39,6 @@ class VPlanVerifier {
// VPHeaderPHIRecipes.
bool verifyPhiRecipes(const VPBasicBlock *VPBB);
- /// Verify that \p EVL is used correctly. The user must be either in
- /// EVL-based recipes as a last operand or VPInstruction::Add which is
- /// incoming value into EVL's recipe.
- bool verifyEVLRecipe(const VPInstruction &EVL) const;
-
/// Verify that \p LastActiveLane's operand is guaranteed to be a prefix-mask.
bool verifyLastActiveLaneRecipe(const VPInstruction &LastActiveLane) const;
@@ -67,9 +61,8 @@ class VPlanVerifier {
bool verifyRegionRec(const VPRegionBlock *Region);
public:
- VPlanVerifier(VPDominatorTree &VPDT, VPTypeAnalysis &TypeInfo,
- bool VerifyLate)
- : VPDT(VPDT), TypeInfo(TypeInfo), VerifyLate(VerifyLate) {}
+ VPlanVerifier(VPDominatorTree &VPDT, VPTypeAnalysis &TypeInfo)
+ : VPDT(VPDT), TypeInfo(TypeInfo) {}
bool verify(const VPlan &Plan);
};
@@ -124,7 +117,7 @@ bool VPlanVerifier::verifyPhiRecipes(const VPBasicBlock *VPBB) {
RecipeI++;
}
- if (!VerifyLate && NumActiveLaneMaskPhiRecipes > 1) {
+ if (!VPBB->getPlan()->isUnrolled() && NumActiveLaneMaskPhiRecipes > 1) {
errs() << "There should be no more than one VPActiveLaneMaskPHIRecipe";
return false;
}
@@ -146,106 +139,19 @@ bool VPlanVerifier::verifyPhiRecipes(const VPBasicBlock *VPBB) {
return true;
}
-bool VPlanVerifier::verifyEVLRecipe(const VPInstruction &EVL) const {
- if (EVL.getOpcode() != VPInstruction::ExplicitVectorLength) {
- errs() << "verifyEVLRecipe should only be called on "
- "VPInstruction::ExplicitVectorLength\n";
- return false;
- }
- auto VerifyEVLUse = [&](const VPRecipeBase &R,
- const unsigned ExpectedIdx) -> bool {
- SmallVector<const VPValue *> Ops(R.operands());
- unsigned UseCount = count(Ops, &EVL);
- if (UseCount != 1 || Ops[ExpectedIdx] != &EVL) {
- errs() << "EVL is used as non-last operand in EVL-based recipe\n";
- return false;
- }
+static bool isKnownMonotonic(VPValue *V) {
+ VPValue *X, *Y;
+ if (match(V, m_Add(m_VPValue(X), m_VPValue(Y))))
+ return isKnownMonotonic(X) && isKnownMonotonic(Y);
+ if (match(V, m_StepVector()))
return true;
- };
- auto VerifyEVLUseInVecEndPtr = [&EVL](auto &VEPRs) {
- if (all_of(VEPRs, [&EVL](VPUser *U) {
- auto *VEPR = cast<VPVectorEndPointerRecipe>(U);
- return match(VEPR->getOffset(),
- m_c_Mul(m_SpecificSInt(VEPR->getStride()),
- m_Sub(m_Specific(&EVL), m_One())));
- }))
- return true;
- errs() << "Expected VectorEndPointer with EVL operand\n";
- return false;
- };
- return all_of(EVL.users(), [&](VPUser *U) {
- return TypeSwitch<const VPUser *, bool>(U)
- .Case([&](const VPWidenIntrinsicRecipe *S) {
- return VerifyEVLUse(*S, S->getNumOperands() - 1);
- })
- .Case<VPWidenStoreEVLRecipe, VPReductionEVLRecipe,
- VPWidenIntOrFpInductionRecipe, VPWidenPointerInductionRecipe>(
- [&](const VPRecipeBase *S) { return VerifyEVLUse(*S, 2); })
- .Case([&](const VPScalarIVStepsRecipe *R) {
- if (R->getNumOperands() != 3) {
- errs() << "Unrolling with EVL tail folding not yet supported\n";
- return false;
- }
- return VerifyEVLUse(*R, 2);
- })
- .Case<VPWidenLoadEVLRecipe, VPVectorEndPointerRecipe,
- VPInterleaveEVLRecipe>(
- [&](const VPRecipeBase *R) { return VerifyEVLUse(*R, 1); })
- .Case(
- [&](const VPInstructionWithType *S) { return VerifyEVLUse(*S, 0); })
- .Case([&](const VPInstruction *I) {
- if (I->getOpcode() == Instruction::PHI ||
- I->getOpcode() == Instruction::ICmp)
- return VerifyEVLUse(*I, 1);
- if (I->getOpcode() == Instruction::Sub) {
- // If Sub has a single user that's a SingleDefRecipe (which is
- // expected to be a Mul), filter its users, in turn, to get
- // VectorEndPointerRecipes, and verify that all the offsets match
- // (EVL - 1) * Stride.
- if (auto *Def = dyn_cast_if_present<VPSingleDefRecipe>(
- I->getSingleUser())) {
- auto VEPRs = make_filter_range(Def->users(),
- IsaPred<VPVectorEndPointerRecipe>);
- if (!VEPRs.empty())
- return VerifyEVLUseInVecEndPtr(VEPRs);
- }
- return VerifyEVLUse(*I, 1);
- }
- switch (I->getOpcode()) {
- case Instruction::Add:
- break;
- case Instruction::UIToFP:
- case Instruction::Trunc:
- case Instruction::ZExt:
- case Instruction::Mul:
- case Instruction::Shl:
- case Instruction::FMul:
- case VPInstruction::Broadcast:
- case VPInstruction::PtrAdd:
- // Opcodes above can only use EVL after wide inductions have been
- // expanded.
- if (!VerifyLate) {
- errs() << "EVL used by unexpected VPInstruction\n";
- return false;
- }
- break;
- default:
- errs() << "EVL used by unexpected VPInstruction\n";
- return false;
- }
- if (!VerifyLate &&
- !isa<VPCurrentIterationPHIRecipe>(*I->users().begin())) {
- errs() << "Result of VPInstruction::Add with EVL operand is "
- "not used by VPCurrentIterationPHIRecipe\n";
- return false;
- }
- return true;
- })
- .Default([&](const VPUser *U) {
- errs() << "EVL has unexpected user\n";
- return false;
- });
- });
+ if (auto *WidenIV = dyn_cast<VPWidenIntOrFpInductionRecipe>(V))
+ return match(WidenIV->getStepValue(), m_One());
+ if (auto *WidenIV = dyn_cast<VPScalarIVStepsRecipe>(V))
+ return match(WidenIV->getStepValue(), m_One());
+ if (isa<VPWidenCanonicalIVRecipe>(V))
+ return true;
+ return vputils::isUniformAcrossVFsAndUFs(V);
}
bool VPlanVerifier::verifyLastActiveLaneRecipe(
@@ -259,18 +165,19 @@ bool VPlanVerifier::verifyLastActiveLaneRecipe(
}
const VPlan &Plan = *LastActiveLane.getParent()->getPlan();
- // All operands must be prefix-mask. Currently we check for header masks or
- // EVL-derived masks, as those are currently the only operands in practice,
- // but this may need updating in the future.
+ // All operands must be prefix-mask. This means an icmp ult/ule LHS, RHS where
+ // the LHS is monotonically increasing and RHS is uniform.
for (VPValue *Op : LastActiveLane.operands()) {
if (vputils::isHeaderMask(Op, Plan))
continue;
- // Masks derived from EVL are also fine.
- auto BroadcastOrEVL =
- m_CombineOr(m_Broadcast(m_EVL(m_VPValue())), m_EVL(m_VPValue()));
- if (match(Op, m_CombineOr(m_ICmp(m_StepVector(), BroadcastOrEVL),
- m_ICmp(BroadcastOrEVL, m_StepVector()))))
+ CmpPredicate Pred;
+ VPValue *LHS, *RHS;
+ if (match(Op, m_ICmp(Pred, m_VPValue(LHS), m_VPValue(RHS))) &&
+ (Pred == CmpInst::ICMP_ULE || Pred == CmpInst::ICMP_ULT) &&
+ isKnownMonotonic(LHS) &&
+ (vputils::isUniformAcrossVFsAndUFs(RHS) ||
+ vputils::isSingleScalar(RHS)))
continue;
errs() << "LastActiveLane operand ";
@@ -372,12 +279,6 @@ bool VPlanVerifier::verifyVPBasicBlock(const VPBasicBlock *VPBB) {
}
if (const auto *VPI = dyn_cast<VPInstruction>(&R)) {
switch (VPI->getOpcode()) {
- case VPInstruction::ExplicitVectorLength:
- if (!verifyEVLRecipe(*VPI)) {
- errs() << "EVL VPValue is not used correctly\n";
- return false;
- }
- break;
case VPInstruction::LastActiveLane:
if (!verifyLastActiveLaneRecipe(*VPI))
return false;
@@ -569,9 +470,9 @@ bool VPlanVerifier::verify(const VPlan &Plan) {
return true;
}
-bool llvm::verifyVPlanIsValid(const VPlan &Plan, bool VerifyLate) {
+bool llvm::verifyVPlanIsValid(const VPlan &Plan) {
VPDominatorTree VPDT(const_cast<VPlan &>(Plan));
VPTypeAnalysis TypeInfo(Plan);
- VPlanVerifier Verifier(VPDT, TypeInfo, VerifyLate);
+ VPlanVerifier Verifier(VPDT, TypeInfo);
return Verifier.verify(Plan);
}
diff --git a/llvm/lib/Transforms/Vectorize/VPlanVerifier.h b/llvm/lib/Transforms/Vectorize/VPlanVerifier.h
index ccf79e8e5c985..642b25b0635c0 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanVerifier.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanVerifier.h
@@ -29,16 +29,14 @@
namespace llvm {
class VPlan;
-/// Verify invariants for general VPlans. If \p VerifyLate is passed, skip some
-/// checks that are not applicable at later stages of the transform pipeline.
+/// Verify invariants for general VPlans.
/// Currently it checks the following:
/// 1. Region/Block verification: Check the Region/Block verification
/// invariants for every region in the H-CFG.
/// 2. all phi-like recipes must be at the beginning of a block, with no other
/// recipes in between. Note that currently there is still an exception for
/// VPBlendRecipes.
-LLVM_ABI_FOR_TEST bool verifyVPlanIsValid(const VPlan &Plan,
- bool VerifyLate = false);
+LLVM_ABI_FOR_TEST bool verifyVPlanIsValid(const VPlan &Plan);
} // namespace llvm
|
fhahn
reviewed
Feb 19, 2026
Comment on lines
+75
to
+77
| if (VerifyEachVPlan && EnableVerify) { | ||
| [[maybe_unused]] bool IsValid = verifyVPlanIsValid(Plan); | ||
| assert(IsValid && "VPlan is invalid"); |
Contributor
There was a problem hiding this comment.
I think the verifier also works in release builds, can we just always exit on failure?
Contributor
Author
There was a problem hiding this comment.
Switched to report_fatal_error in 9401b48. I copied the same error format from the IR verifier.
eas
reviewed
Feb 19, 2026
eas
reviewed
Feb 19, 2026
eas
reviewed
Feb 19, 2026
Contributor
eas
left a comment
eas
left a comment
There was a problem hiding this comment.
Nothing else from me, thanks!
fhahn
reviewed
Feb 19, 2026
Comment on lines
+144
to
+145
| if (match(V, m_Add(m_VPValue(X), m_VPValue(Y)))) | ||
| return isKnownMonotonic(X) && isKnownMonotonic(Y); |
Contributor
There was a problem hiding this comment.
I think this would also need to check that the Add is NUW, if it would wrap it would not be monotonic, even if both operands are
Contributor
Author
There was a problem hiding this comment.
Looks like this actually causes verifier failures in test/Transforms/LoopVectorize/first-order-recurrence-tail-folding.ll, %10 is used as a prefix mask in a lastactivelane but %step.add doesn't have unsigned wrap:
# | VPlan 'Initial VPlan for VF={2},UF={2}' {
# | Live-in vp<%0> = VF
# | Live-in vp<%1> = VF * UF
# | Live-in vp<%2> = vector-trip-count
# | Live-in vp<%3> = backedge-taken count
# | Live-in ir<%n> = original trip-count
# |
# | ir-bb<entry>:
# | EMIT branch-on-cond ir<false>
# | Successor(s): scalar.ph, vector.ph
# |
# | vector.ph:
# | EMIT vp<%5> = wide-iv-step vp<%0>, ir<1>
# | Successor(s): vector loop
# |
# | <x1> vector loop: {
# | vector.body:
# | EMIT vp<%6> = CANONICAL-INDUCTION ir<0>, vp<%index.next>
# | ir<%iv> = WIDEN-INDUCTION ir<0>, ir<1>, vp<%0>, vp<%5>, vp<%step.add>
# | EMIT vp<%step.add> = add ir<%iv>, vp<%5>
# | EMIT vp<%10> = icmp ule vp<%step.add>, vp<%3>
Will take a look to see if we're missing a nuw on the step.add from unrolling.
Contributor
There was a problem hiding this comment.
would be good to add a test case; in general, if the wide IV had nuw, I * think* that should also be preserve-able for the steps, and if it was canonical
Contributor
Author
There was a problem hiding this comment.
I think so too. However I'm noticing we actually drop the nuw on the wide IV after #163538. I believe the original reasoning was that with tail folding we might end up with poison lanes because the VTC > TC. But I don't think those lanes are ever used anyway. They're used for computing the header mask, which can't have any poison lanes.
Contributor
Author
There was a problem hiding this comment.
I went down a bit of a rabbit hole here. IIUC we check for the canonical IV overflowing with tail folding because vscale can be a non-power-of-2.
AFAIK the plan is to remove non-power-of-2 vscales in #145098, and if that lands we can remove all the overflow checks so the increments will always have NUW.
Until then I think it's quite tricky to infer NUW. Can we relax the NUW check here and add a TODO to add it back if/when #145098 lands?
david-arm
reviewed
Feb 20, 2026
|
|
||
| assert(verifyVPlanIsValid(BestVPlan, true /*VerifyLate*/) && | ||
| "final VPlan is invalid"); | ||
| assert(verifyVPlanIsValid(BestVPlan) && "final VPlan is invalid"); |
Contributor
There was a problem hiding this comment.
How come this isn't also guarded by EnableVerify like in the vplan code? I was expecting something similar here:
bool VPlanIsValid = verifyVPlanIsValid(BestVPlan);
if (EnableVerify && !VPlanIsValid)
report_fatal_error("Broken VPlan found, compilation aborted!");
else
assert(VPlanIsValid && "final VPlan is invalid");
Contributor
There was a problem hiding this comment.
This is just an assert that verifies that the plan is valid at this specific point, independent of running the verifier after all transforms. This should mirror how verification is handled for LLVM IR verification
Contributor
There was a problem hiding this comment.
But in a release build does the IR verifier report a fatal error for unverified IR? If so, then if we mirror that behaviour this should also be a fatal error.
Contributor
There was a problem hiding this comment.
Yep, verifyFunction for IR similarly just returns a boolean, instead of reporting a fatal error:
llvm-project/llvm/lib/IR/Verifier.cpp
Line 7885 in 58ac25e
It is used in asserts so those won't raise a fatal error in release builds (e.g. the assert used in LV). If the verifier is enabled to run after each pass, that raises a fatal error.
| // All operands must be prefix-mask. Currently we check for header masks or | ||
| // EVL-derived masks, as those are currently the only operands in practice, | ||
| // but this may need updating in the future. | ||
| // All operands must be prefix-mask. This means an icmp ult/ule LHS, RHS where |
Contributor
There was a problem hiding this comment.
Can the changes in this function be done in a separate PR or are they explicitly tied to the removal of the 'VerifyLate' parameter?
Contributor
Author
There was a problem hiding this comment.
The VerifyLate parameter bit was split off and landed in #182799
lukel97
added a commit
to lukel97/llvm-project
that referenced
this pull request
Feb 23, 2026
In llvm#182254 we want to start aborting compilation when the verifier fails between passes, but currently we run into various EVL related failures. The EVL is used in quite a few more places than when the verification was originally added, all of which need to be handled by the verifier. I think this is also exacerbated by the fact that many recipes nowadays are converted to concrete recipes later in the pipeline which duplicates the number of patterns we need to match. The EVL transform itself has also changed much since its original implementation, i.e. non-trapping recipes don't use EVL (llvm#127180) and VP recipes are generated via pattern matching instead of unconditionally (llvm#155394), so I'm not sure if the verification is as relevant today. Rather than try to add more patterns this PR removes the verification to reduce the maintainence cost. Split off from llvm#182254
lukel97
added a commit
that referenced
this pull request
Feb 23, 2026
In #182254 we want to start aborting compilation when the verifier fails between passes, but currently we run into various EVL related failures. The EVL is used in quite a few more places than when the verification was originally added, all of which need to be handled by the verifier. I think this is also exacerbated by the fact that many recipes nowadays are converted to concrete recipes later in the pipeline which duplicates the number of patterns we need to match. The EVL transform itself has also changed much since its original implementation, i.e. non-trapping recipes don't use EVL (#127180) and VP recipes are generated via pattern matching instead of unconditionally (#155394), so I'm not sure if the verification is as relevant today. Rather than try to add more patterns this PR removes the verification to reduce the maintainence cost. Split off from #182254
…ze/vplan-verify-assert
Contributor
Author
|
Rebased on the split off parts so now should only contain the report_fatal_error bit and the LastActiveLane fixes |
lukel97
changed the title
🐧 Linux x64 Test Results
Failed Tests(click on a test name to see its output) lldb-apilldb-api.tools/lldb-dap/progress/TestDAP_Progress.pyIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
fhahn
reviewed
Feb 23, 2026
Contributor
fhahn
left a comment
fhahn
left a comment
There was a problem hiding this comment.
Thanks for splitting the changes off! I'll run this on some large test cases overnight to double-check there are no other verification issues this would highlight.
fhahn
reviewed
Feb 24, 2026
| if (match(Op, m_ICmp(Pred, m_VPValue(LHS), m_VPValue(RHS))) && | ||
| (Pred == CmpInst::ICMP_ULE || Pred == CmpInst::ICMP_ULT) && | ||
| isKnownMonotonic(LHS) && | ||
| (vputils::isUniformAcrossVFsAndUFs(RHS) || |
Contributor
There was a problem hiding this comment.
Is the only case that it missing from vputils::isUniformAcrossVFsAndUFs(RHS) EVL? Should EVL be considered uniform-across-VF-and-UFs? Currently we never unroll with EVL so that should be fine I think
Contributor
Author
There was a problem hiding this comment.
Yeah it's just EVL. I think it would be nice to avoid having the UF = 1 invariant in isUniformAcrossVFsAndUFs for EVL though. I just restricted the isSingleScalar to EVL in the verifier instead in b503231 if that works for you
fhahn
approved these changes
Feb 24, 2026
| // EVL-derived masks, as those are currently the only operands in practice, | ||
| // but this may need updating in the future. | ||
| // All operands must be prefix-mask. This means an icmp ult/ule LHS, RHS where | ||
| // the LHS is monotonically increasing and RHS is uniform. |
Contributor
There was a problem hiding this comment.
Suggested change
| // the LHS is monotonically increasing and RHS is uniform. | |
| // the LHS is monotonically increasing and RHS is uniform across VF and UF. |
Comment on lines
+76
to
+77
| if (!verifyVPlanIsValid(Plan)) | ||
| report_fatal_error("Broken VPlan found, compilation aborted!"); |
Contributor
There was a problem hiding this comment.
If possible, it may be good to add a C++ unit test that passes an invalid VPlan to a transform invoked via the macro, to add test coverage for the reporting.
…ze/vplan-verify-assert
…ze/vplan-verify-assert
sujianIBM
pushed a commit
to sujianIBM/llvm-project
that referenced
this pull request
Mar 5, 2026
…cation (llvm#182254) Currently if -vplan-verify-each is enabled and a pass fails the verifier, it will output the failure to stderr but will still finish with a zero exit code. This adds an assert that the verification fails so that e.g. lit will pick up verifier failures in the in-tree tests with an EXPENSIVE_CHECKS build. Currently the LastActiveLane verification fails in several tests, so this also includes a fix to handle more prefix masks. All of the prefix masks that the verifier encounters are of the form `icmp ult/ule monotonically-increasing-sequence, uniform`, which always generate a prefix mask. Tested that llvm-test-suite + SPEC CPU 2017 now pass with -vplan-verify-each enabled for RISC-V.
HendrikHuebner
pushed a commit
to HendrikHuebner/llvm-project
that referenced
this pull request
Mar 10, 2026
In llvm#182254 we want to start aborting compilation when the verifier fails between passes, but currently we run into various EVL related failures. The EVL is used in quite a few more places than when the verification was originally added, all of which need to be handled by the verifier. I think this is also exacerbated by the fact that many recipes nowadays are converted to concrete recipes later in the pipeline which duplicates the number of patterns we need to match. The EVL transform itself has also changed much since its original implementation, i.e. non-trapping recipes don't use EVL (llvm#127180) and VP recipes are generated via pattern matching instead of unconditionally (llvm#155394), so I'm not sure if the verification is as relevant today. Rather than try to add more patterns this PR removes the verification to reduce the maintainence cost. Split off from llvm#182254
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Currently if -vplan-verify-each is enabled and a pass fails the verifier, it will output the failure to stderr but will still finish with a zero exit code.
This adds an assert that the verification fails so that e.g. lit will pick up verifier failures in the in-tree tests with an EXPENSIVE_CHECKS build.
Currently the LastActiveLane verification fails in several tests, so this also includes a fix to handle more prefix masks. All of the prefix masks that the verifier encounters are of the form
icmp ult/ule monotonically-increasing-sequence, uniform, which always generate a prefix mask.Tested that llvm-test-suite + SPEC CPU 2017 now pass with -vplan-verify-each enabled for RISC-V.