[LV] Don't vectorize epilogue with scalable VF if no iterations remain. #149789
+436
−251
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4479,6 +4479,28 @@ VectorizationFactor LoopVectorizationPlanner::selectEpilogueVectorizationFactor( | |
| Type *TCType = Legal->getWidestInductionType(); | ||
| const SCEV *RemainingIterations = nullptr; | ||
| unsigned MaxTripCount = 0; | ||
| if (MainLoopVF.isFixed()) { | ||
|
There was a problem hiding this comment. I've a few SCEV patches, one at review and one delayed by #146102 that should fix this. That said, we should not be unnecessarily restricting things to fixed length vectors. As a minimum I think the There was a problem hiding this comment. Yep I have a patch to remove the restriction, although it adds new functionality independent of the change, will share separately. |
||
| // TODO: extend to support scalable VFs. | ||
| const SCEV *TC = vputils::getSCEVExprForVPValue( | ||
| getPlanFor(MainLoopVF).getTripCount(), SE); | ||
| assert(!isa<SCEVCouldNotCompute>(TC) && | ||
| "Trip count SCEV must be computable"); | ||
| RemainingIterations = SE.getURemExpr( | ||
| TC, SE.getConstant(TCType, MainLoopVF.getFixedValue() * IC)); | ||
|
|
||
| // No iterations left to process in the epilogue. | ||
| if (RemainingIterations->isZero()) | ||
| return Result; | ||
|
|
||
| MaxTripCount = MainLoopVF.getFixedValue() * IC - 1; | ||
| if (SE.isKnownPredicate(CmpInst::ICMP_ULT, RemainingIterations, | ||
| SE.getConstant(TCType, MaxTripCount))) { | ||
| MaxTripCount = SE.getUnsignedRangeMax(RemainingIterations).getZExtValue(); | ||
| } | ||
| LLVM_DEBUG(dbgs() << "LEV: Maximum Trip Count for Epilogue: " | ||
| << MaxTripCount << "\n"); | ||
| } | ||
|
|
||
| for (auto &NextVF : ProfitableVFs) { | ||
| // Skip candidate VFs without a corresponding VPlan. | ||
| if (!hasPlanWithVF(NextVF.Width)) | ||
|
|
@@ -4496,24 +4518,7 @@ VectorizationFactor LoopVectorizationPlanner::selectEpilogueVectorizationFactor( | |
|
|
||
| // If NextVF is greater than the number of remaining iterations, the | ||
| // epilogue loop would be dead. Skip such factors. | ||
| if (RemainingIterations && !NextVF.Width.isScalable()) { | ||
|
There was a problem hiding this comment. The |
||
| if (SE.isKnownPredicate( | ||
| CmpInst::ICMP_UGT, | ||
| SE.getConstant(TCType, NextVF.Width.getFixedValue()), | ||
|
|
||
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
Oops, something went wrong.
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.
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.
With the work done by @paulwalker-arm (see #146102) I think we also need to do this for scalable main loop VFs too. For example, the trip count may be a multiple of vscale such as 16 * vscale, and if the main loop VF=vscale x 4 with IC = 1, we know at compile there will be no remaining iterations.
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.
Yep, we may also be able to improve this also for cases where we know what vscale to optimize for, as this is a profitability check.
There's already an existing TODO to support scalable VFs here
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.
I actually wasn't thinking about performance - I was trying to say that this patch may only be a partial fix, i.e. in the example I mentioned the remaining iterations is known to be zero and so won't we hit the same assert?
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.
Oh I see. Once we support that, we need to update the code here.
The reason for the assert is that
simplifyBranchConditionForVFAndUFcan remove the vector loop branch, which in turn means we cannot recover the induction resume value for the epilogue loop, because none exits. I could also try to fix that, but it will quickly become dead code.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.
Hmm, I tried #146102 in combination with this PR and ran
opt -p loop-vectorize -force-vector-interleave=1 -epilogue-vectorization-minimum-VF=2 -S < ../llvm/test/Transforms/LoopVectorize/AArch64/sve-vscale-based-trip-counts.lland I didn't hit any errors even though there are no remaining iterations in testvscale_mul_4. Perhaps the vscale multiplier still gets in the way somewhere.