-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[LV] Add a statistic for early exit vectorization #145730
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
Conversation
@llvm/pr-subscribers-vectorizers @llvm/pr-subscribers-llvm-transforms Author: None (annamthomas) ChangesWe currently do not vectorize the epilog loops with early-exits, but the Full diff: https://github.com/llvm/llvm-project/pull/145730.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 5eda2003920e6..4f365b9f990bf 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -175,6 +175,8 @@ const char LLVMLoopVectorizeFollowupEpilogue[] =
STATISTIC(LoopsVectorized, "Number of loops vectorized");
STATISTIC(LoopsAnalyzed, "Number of loops analyzed for vectorization");
STATISTIC(LoopsEpilogueVectorized, "Number of epilogues vectorized");
+STATISTIC(LoopsEarlyExitVectorized, "Number of early exit loops vectorized");
+
static cl::opt<bool> EnableEpilogueVectorization(
"enable-epilogue-vectorization", cl::init(true), cl::Hidden,
@@ -10291,6 +10293,9 @@ bool LoopVectorizePass::processLoop(Loop *L) {
auto ExpandedSCEVs = LVP.executePlan(EPI.MainLoopVF, EPI.MainLoopUF,
*BestMainPlan, MainILV, DT, false);
++LoopsVectorized;
+ // TODO: Currently unsupported for early exits.
+ if (BestPlan.hasEarlyExit())
+ ++LoopsEarlyExitVectorized;
// Second pass vectorizes the epilogue and adjusts the control flow
// edges from the first pass.
@@ -10319,6 +10324,10 @@ bool LoopVectorizePass::processLoop(Loop *L) {
Inc->setIncomingValueForBlock(BypassBlock, V);
}
++LoopsEpilogueVectorized;
+ // TODO: Currently unsupported for early exits.
+ if (BestEpiPlan.hasEarlyExit())
+ ++LoopsEarlyExitVectorized;
+
if (!Checks.hasChecks())
DisableRuntimeUnroll = true;
@@ -10328,6 +10337,8 @@ bool LoopVectorizePass::processLoop(Loop *L) {
Checks, BestPlan);
LVP.executePlan(VF.Width, IC, BestPlan, LB, DT, false);
++LoopsVectorized;
+ if (BestPlan.hasEarlyExit())
+ ++LoopsEarlyExitVectorized;
// Add metadata to disable runtime unrolling a scalar loop when there
// are no runtime checks about strides and memory. A scalar loop that is
diff --git a/llvm/test/Transforms/LoopVectorize/single_early_exit.ll b/llvm/test/Transforms/LoopVectorize/single_early_exit.ll
index 00e21f102038a..c7b4a8f915405 100644
--- a/llvm/test/Transforms/LoopVectorize/single_early_exit.ll
+++ b/llvm/test/Transforms/LoopVectorize/single_early_exit.ll
@@ -1,7 +1,7 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
; RUN: opt -S < %s -p loop-vectorize -enable-early-exit-vectorization -force-vector-width=4 | FileCheck %s
-declare void @init_mem(ptr, i64);
+eclare void @init_mem(ptr, i64);
define i64 @same_exit_block_phi_of_consts() {
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
Might also be worth adding a test for the new statistic
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.
LGTM, left a few suggestions to simplify the test case, as it seems like a slightly simpler one would suffice.
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.
=1
shouldn't be needed because it's the default now, but I guess it also doesn't hurt.
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.
might as well use a simpler case, with constant here, i64 compare to remove the unnecessary zext.
Also can use a single IV?
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.
Looks like there are some test failures, possibly for cases when there are no exit blocks (perhaps for interleave groups which require the scalar epilogue?)
Suspect we may need to check if there's at least one exit block in hasEarlyExit
Yep, it is failing for exactly that. What's happening is that we are vectorizing the scalar epilog loop and there are no exit blocks for that scalar epilog loop. So, we fail. |
If we call this API during vectorization without any exit blocks, we need to first check there is atleast one exit block.
Add statistic LoopsEarlyExitVectorized
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.
LGTM with comment addressed!
@@ -1,12 +1,12 @@ | |||
; RUN: opt < %s -passes=loop-vectorize -force-vector-interleave=4 -force-vector-width=4 -debug-only=loop-vectorize --disable-output -stats -S 2>&1 | FileCheck %s | |||
; RUN: opt < %s -passes=loop-vectorize -force-vector-interleave=4 -force-vector-width=4 -debug-only=loop-vectorize -enable-early-exit-vectorization --disable-output -stats -S 2>&1 | FileCheck %s |
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.
The flag enable-early-exit-vectorization
can be removed now, since it's on by default.
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, @david-arm, I actually prefer to have the option left in for 2 reasons:
- Easier to see which lit tests are affected by early-exit vectorization. We have a bunch of them with the option on and it helps to see what tests already exist.
- after the option is baked-in for a release or so, it makes sense to remove the option for all the tests, since we don't want to keep adding ON-by-default options in tests
Thoughts?
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.
LGTM, thanks
CI Failure unrelated to PR. Its an LSR failure. I'll be landing the changes manually since there are 2 changes in this PR. |
If we call this API during vectorization without any exit blocks, we need to first check there is atleast one exit block. PR: #145730
Add statistic LoopsEarlyExitVectorized PR: #145730
Commits landed manually to main. |
…k case If we call this API during vectorization without any exit blocks, we need to first check there is atleast one exit block. PR: llvm/llvm-project#145730
Add statistic LoopsEarlyExitVectorized PR: llvm/llvm-project#145730
We currently do not vectorize the epilog loops with early-exits, but the
stats are updated there as well for completeness.