Skip to content

[VPlan] Assert vplan-verify-each result and fix LastActiveLane verification #182254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lukel97 merged 12 commits into from
Feb 25, 2026
Merged

[VPlan] Assert vplan-verify-each result and fix LastActiveLane verification #182254

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
896ef73
[VPlan] Assert vplan-verify-each result and get verifier passing
lukel97 Feb 19, 2026
9401b48
Use report_fatal_error
lukel97 Feb 19, 2026
e9059ef
Tighten monotonic IV checks to cases where there's no known overflow
lukel97 Feb 19, 2026
ba01b63
Add comment
lukel97 Feb 19, 2026
db8dbb3
Check for nuw on add
lukel97 Feb 20, 2026
04e4ee7
Merge branch 'main' of github.com:llvm/llvm-project into loop-vectori…
lukel97 Feb 23, 2026
08155c7
Fix accidentally dropping recursive m_Add monotonic check, use isCano…
lukel97 Feb 24, 2026
b503231
Remove noUnsignedWrap check, check for EVL explicitly
lukel97 Feb 24, 2026
90160d8
Merge branch 'main' of github.com:llvm/llvm-project into loop-vectori…
lukel97 Feb 25, 2026
0f62428
Update comment, add unit test
lukel97 Feb 25, 2026
bb065bb
Merge branch 'main' of github.com:llvm/llvm-project into loop-vectori…
lukel97 Feb 25, 2026
84eb197
Undo stray whitespace change
lukel97 Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,6 +859,8 @@ inline auto m_c_LogicalOr(const Op0_t &Op0, const Op1_t &Op1) {
return m_c_Select(Op0, m_True(), Op1);
}

inline auto m_CanonicalIV() { return class_match<VPCanonicalIVPHIRecipe>(); }

template <typename Op0_t, typename Op1_t, typename Op2_t>
using VPScalarIVSteps_match = Recipe_match<std::tuple<Op0_t, Op1_t, Op2_t>, 0,
false, VPScalarIVStepsRecipe>;
Expand Down
6 changes: 4 additions & 2 deletions llvm/lib/Transforms/Vectorize/VPlanTransforms.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,10 @@ struct VPlanTransforms {
dbgs() << Plan << '\n';
}
#endif
if (VerifyEachVPlan && EnableVerify)
verifyVPlanIsValid(Plan);
if (VerifyEachVPlan && EnableVerify) {
if (!verifyVPlanIsValid(Plan))
report_fatal_error("Broken VPlan found, compilation aborted!");
Comment on lines +76 to +77

@fhahn fhahn Feb 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lukel97 lukel97 Feb 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 0f62428

}
}};

return std::forward<PassTy>(Pass)(Plan, std::forward<ArgsTy>(Args)...);
Expand Down
38 changes: 30 additions & 8 deletions llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,27 @@ bool VPlanVerifier::verifyPhiRecipes(const VPBasicBlock *VPBB) {
return true;
}

static bool isKnownMonotonic(VPValue *V) {
VPValue *X, *Y;
// TODO: Check for hasNoUnsignedWrap() when we set nuw in VPlanUnroll
if (match(V, m_Add(m_VPValue(X), m_VPValue(Y))))
return isKnownMonotonic(X) && isKnownMonotonic(Y);
Comment on lines +145 to +146

@fhahn fhahn Feb 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lukel97 lukel97 Feb 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in db8dbb3

@lukel97 lukel97 Feb 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@fhahn fhahn Feb 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lukel97 lukel97 Feb 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lukel97 lukel97 Feb 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if (match(V, m_StepVector()))
return true;
// Only handle a subset of IVs until we can guarantee there's no overflow.
if (auto *WidenIV = dyn_cast<VPWidenIntOrFpInductionRecipe>(V))
Comment thread
eas marked this conversation as resolved.
return WidenIV->isCanonical();
if (auto *Steps = dyn_cast<VPScalarIVStepsRecipe>(V))
return match(Steps->getOperand(0),
m_CombineOr(
m_CanonicalIV(),
m_DerivedIV(m_ZeroInt(), m_CanonicalIV(), m_One()))) &&
match(Steps->getStepValue(), m_One());
if (isa<VPWidenCanonicalIVRecipe>(V))
return true;
return vputils::isUniformAcrossVFsAndUFs(V);
}

bool VPlanVerifier::verifyLastActiveLaneRecipe(
const VPInstruction &LastActiveLane) const {
assert(LastActiveLane.getOpcode() == VPInstruction::LastActiveLane &&
Expand All @@ -150,18 +171,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

@david-arm david-arm Feb 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the changes in this function be done in a separate PR or are they explicitly tied to the removal of the 'VerifyLate' parameter?

@lukel97 lukel97 Feb 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The VerifyLate parameter bit was split off and landed in #182799

// the LHS is monotonically increasing and RHS is uniform.

@fhahn fhahn Feb 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// the LHS is monotonically increasing and RHS is uniform.
// the LHS is monotonically increasing and RHS is uniform across VF and UF.

@lukel97 lukel97 Feb 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 0f62428

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) ||

@fhahn fhahn Feb 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lukel97 lukel97 Feb 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

match(RHS, m_EVL(m_VPValue()))))
continue;

errs() << "LastActiveLane operand ";
Expand Down