-
Notifications
You must be signed in to change notification settings - Fork 16.8k
[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.
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
Changes from all commits
896ef73
9401b48
e9059ef
ba01b63
db8dbb3
04e4ee7
08155c7
b503231
90160d8
0f62428
bb065bb
84eb197
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
| 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)) | ||
eas marked this conversation as resolved.
Show resolved
Hide 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 && | ||
|
|
@@ -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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 across VFs and UF. | ||
| 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) || | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the only case that it missing from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 "; | ||
|
|
||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 0f62428