[VPlan] Verify plan before optimizations. NFC #122678
Merged
Conversation
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
I've been exploring verifying the VPlan before and after the EVL transformation steps, and noticed that the VPlan comes out in an invalid state between construction and optimisation.
In adjustRecipesForReductions, we leave behind some dead recipes which are invalid:
1) When we replace a link with a reduction recipe, the old link ends up becoming a use-before-def:
WIDEN ir<%l7> = add ir<%sum.02>, ir<%indvars.iv>.1
WIDEN ir<%l8> = add ir<%l7>.1, ir<%l3>
WIDEN ir<%l9> = add ir<%l8>.1, ir<%l5>
...
REDUCE ir<%l7>.1 = ir<%sum.02> + reduce.add (ir<%indvars.iv>.1)
REDUCE ir<%l8>.1 = ir<%l7>.1 + reduce.add (ir<%l3>)
REDUCE ir<%l9>.1 = ir<%l8>.1 + reduce.add (ir<%l5>)
2) When transforming an AnyOf reduction phi to a boolean, we leave behind a select with mismatching operand types, which triggers the assertions in VTypeAnalysis
This adds an extra verification step and deletes the dead recipes eagerly to keep the plan valid.
|
@llvm/pr-subscribers-llvm-transforms Author: Luke Lau (lukel97) ChangesI've been exploring verifying the VPlan before and after the EVL transformation steps, and noticed that the VPlan comes out in an invalid state between construction and optimisation. In adjustRecipesForReductions, we leave behind some dead recipes which are invalid:
This adds an extra verification step and deletes the dead recipes eagerly to keep the plan valid. Full diff: https://github.com/llvm/llvm-project/pull/122678.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index d32a463a996c4f..5bd4ae5e0a442b 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -8837,6 +8837,8 @@ void LoopVectorizationPlanner::buildVPlansWithVPRecipes(ElementCount MinVF,
for (ElementCount VF = MinVF; ElementCount::isKnownLT(VF, MaxVFTimes2);) {
VFRange SubRange = {VF, MaxVFTimes2};
if (auto Plan = tryToBuildVPlanWithVPRecipes(SubRange)) {
+ assert(verifyVPlanIsValid(*Plan) && "VPlan is invalid");
+
// Now optimize the initial VPlan.
if (!Plan->hasVF(ElementCount::getFixed(1)))
VPlanTransforms::truncateToMinimalBitwidths(*Plan,
@@ -9553,6 +9555,8 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
VPRegionBlock *VectorLoopRegion = Plan->getVectorLoopRegion();
VPBasicBlock *Header = VectorLoopRegion->getEntryBasicBlock();
VPBasicBlock *MiddleVPBB = Plan->getMiddleBlock();
+ SmallVector<VPRecipeBase *> ToDelete;
+
for (VPRecipeBase &R : Header->phis()) {
auto *PhiR = dyn_cast<VPReductionPHIRecipe>(&R);
if (!PhiR || !PhiR->isInLoop() || (MinVF.isScalar() && !PhiR->isOrdered()))
@@ -9672,10 +9676,11 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
CM.useOrderedReductions(RdxDesc), CurrentLinkI->getDebugLoc());
// Append the recipe to the end of the VPBasicBlock because we need to
// 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.
+ // Delete CurrentLink as it will be invalid if its operand is replaced
+ // with a reduction defined at the bottom of the block in the next link.
LinkVPBB->appendRecipe(RedRecipe);
CurrentLink->replaceAllUsesWith(RedRecipe);
+ ToDelete.push_back(CurrentLink);
PreviousLink = RedRecipe;
}
}
@@ -9790,6 +9795,8 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
Cmp = Builder.createNot(Cmp);
VPValue *Or = Builder.createOr(PhiR, Cmp);
Select->getVPSingleValue()->replaceAllUsesWith(Or);
+ // Delete Select now that it has invalid types.
+ ToDelete.push_back(Select);
// Convert the reduction phi to operate on bools.
PhiR->setOperand(0, Plan->getOrAddLiveIn(ConstantInt::getFalse(
@@ -9807,6 +9814,8 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
}
VPlanTransforms::clearReductionWrapFlags(*Plan);
+ for (VPRecipeBase *R : ToDelete)
+ R->eraseFromParent();
}
void VPDerivedIVRecipe::execute(VPTransformState &State) {
|
|
@llvm/pr-subscribers-vectorizers Author: Luke Lau (lukel97) ChangesI've been exploring verifying the VPlan before and after the EVL transformation steps, and noticed that the VPlan comes out in an invalid state between construction and optimisation. In adjustRecipesForReductions, we leave behind some dead recipes which are invalid:
This adds an extra verification step and deletes the dead recipes eagerly to keep the plan valid. Full diff: https://github.com/llvm/llvm-project/pull/122678.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index d32a463a996c4f..5bd4ae5e0a442b 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -8837,6 +8837,8 @@ void LoopVectorizationPlanner::buildVPlansWithVPRecipes(ElementCount MinVF,
for (ElementCount VF = MinVF; ElementCount::isKnownLT(VF, MaxVFTimes2);) {
VFRange SubRange = {VF, MaxVFTimes2};
if (auto Plan = tryToBuildVPlanWithVPRecipes(SubRange)) {
+ assert(verifyVPlanIsValid(*Plan) && "VPlan is invalid");
+
// Now optimize the initial VPlan.
if (!Plan->hasVF(ElementCount::getFixed(1)))
VPlanTransforms::truncateToMinimalBitwidths(*Plan,
@@ -9553,6 +9555,8 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
VPRegionBlock *VectorLoopRegion = Plan->getVectorLoopRegion();
VPBasicBlock *Header = VectorLoopRegion->getEntryBasicBlock();
VPBasicBlock *MiddleVPBB = Plan->getMiddleBlock();
+ SmallVector<VPRecipeBase *> ToDelete;
+
for (VPRecipeBase &R : Header->phis()) {
auto *PhiR = dyn_cast<VPReductionPHIRecipe>(&R);
if (!PhiR || !PhiR->isInLoop() || (MinVF.isScalar() && !PhiR->isOrdered()))
@@ -9672,10 +9676,11 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
CM.useOrderedReductions(RdxDesc), CurrentLinkI->getDebugLoc());
// Append the recipe to the end of the VPBasicBlock because we need to
// 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.
+ // Delete CurrentLink as it will be invalid if its operand is replaced
+ // with a reduction defined at the bottom of the block in the next link.
LinkVPBB->appendRecipe(RedRecipe);
CurrentLink->replaceAllUsesWith(RedRecipe);
+ ToDelete.push_back(CurrentLink);
PreviousLink = RedRecipe;
}
}
@@ -9790,6 +9795,8 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
Cmp = Builder.createNot(Cmp);
VPValue *Or = Builder.createOr(PhiR, Cmp);
Select->getVPSingleValue()->replaceAllUsesWith(Or);
+ // Delete Select now that it has invalid types.
+ ToDelete.push_back(Select);
// Convert the reduction phi to operate on bools.
PhiR->setOperand(0, Plan->getOrAddLiveIn(ConstantInt::getFalse(
@@ -9807,6 +9814,8 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
}
VPlanTransforms::clearReductionWrapFlags(*Plan);
+ for (VPRecipeBase *R : ToDelete)
+ R->eraseFromParent();
}
void VPDerivedIVRecipe::execute(VPTransformState &State) {
|
fhahn
reviewed
Jan 13, 2025
| LinkVPBB->appendRecipe(RedRecipe); | ||
| CurrentLink->replaceAllUsesWith(RedRecipe); | ||
| ToDelete.push_back(CurrentLink); |
There was a problem hiding this comment.
Is there anything from preventing us from deleting it straight away here?
There was a problem hiding this comment.
Yeah, it doesn't play well with the for (VPRecipeBase &R : Header->phis()) iterator. And we can't use make_early_inc_range either since we're deleting an instruction that's not the current iterator
fhahn
reviewed
Jan 13, 2025
| @@ -8837,6 +8837,8 @@ void LoopVectorizationPlanner::buildVPlansWithVPRecipes(ElementCount MinVF, | |||
| for (ElementCount VF = MinVF; ElementCount::isKnownLT(VF, MaxVFTimes2);) { | |||
| VFRange SubRange = {VF, MaxVFTimes2}; | |||
| if (auto Plan = tryToBuildVPlanWithVPRecipes(SubRange)) { | |||
| assert(verifyVPlanIsValid(*Plan) && "VPlan is invalid"); | |||
There was a problem hiding this comment.
better do this at the end of tryToBuildVPlanWithVPRecipes?
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/13741 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/6745 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/11935 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I've been exploring verifying the VPlan before and after the EVL transformation steps, and noticed that the VPlan comes out in an invalid state between construction and optimisation.
In adjustRecipesForReductions, we leave behind some dead recipes which are invalid:
When we replace a link with a reduction recipe, the old link ends up becoming a use-before-def:
WIDEN ir<%l7> = add ir<%sum.02>, ir<%indvars.iv>.1
WIDEN ir<%l8> = add ir<%l7>.1, ir<%l3>
WIDEN ir<%l9> = add ir<%l8>.1, ir<%l5>
...
REDUCE ir<%l7>.1 = ir<%sum.02> + reduce.add (ir<%indvars.iv>.1)
REDUCE ir<%l8>.1 = ir<%l7>.1 + reduce.add (ir<%l3>)
REDUCE ir<%l9>.1 = ir<%l8>.1 + reduce.add (ir<%l5>)
When transforming an AnyOf reduction phi to a boolean, we leave behind a select with mismatching operand types, which will trigger the assertions in VTypeAnalysis after [VPlan] Verify scalar types in VPlanVerifier. NFCI #122679
This adds an extra verification step and deletes the dead recipes eagerly to keep the plan valid.