[LoopVectorize] Clear stale CycleAnalysis after loop simplification - #218563
[LoopVectorize] Clear stale CycleAnalysis after loop simplification#218563Opriego wants to merge 1 commit into
Conversation
|
Hello @Opriego 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
|
@llvm/pr-subscribers-vectorizers Author: Oscar Priego (Opriego) ChangesFixes #218392.
If Clear a cached This is similar to #215237, which clears stale Added a reduced regression test based on #218392. Testing:
Full diff: https://github.com/llvm/llvm-project/pull/218563.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 63f955538a8d7..97fd016a63d2c 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -8330,6 +8330,12 @@ LoopVectorizeResult LoopVectorizePass::runImpl(Function &F) {
Changed |= CFGChanged |=
simplifyLoop(L, DT, LI, SE, AC, nullptr, false /* PreserveLCSSA */);
+ // Loop simplification may change the CFG before processLoop() lazily requests
+ // BlockFrequencyAnalysis. Clear a cached CycleAnalysis so BFI recomputes it
+ // for the simplified CFG.
+ if (CFGChanged && FAM->getCachedResult<CycleAnalysis>(F))
+ FAM->clearAnalysis<CycleAnalysis>(F);
+
// Build up a worklist of inner-loops to vectorize. This is necessary as
// the act of vectorizing or partially unrolling a loop creates new loops
// and can invalidate iterators across the loops.
diff --git a/llvm/test/Transforms/LoopVectorize/bfi-stale-after-loop-simplify.ll b/llvm/test/Transforms/LoopVectorize/bfi-stale-after-loop-simplify.ll
new file mode 100644
index 0000000000000..1dfe4882f9193
--- /dev/null
+++ b/llvm/test/Transforms/LoopVectorize/bfi-stale-after-loop-simplify.ll
@@ -0,0 +1,25 @@
+; RUN: opt < %s -passes='function(require<cycles>,loop-vectorize)' -disable-output
+
+; LoopVectorize simplifies loops before processing them. If CycleAnalysis was
+; cached before the pass, it must not be reused after loop simplification
+; changes the CFG when BlockFrequencyInfo is requested lazily.
+
+define i16 @f(i1 %c) {
+entry:
+ br label %header
+
+latch:
+ %iv.next = add i16 %iv, 1
+ %done = icmp eq i16 %iv.next, 0
+ br i1 %done, label %second, label %header
+
+header:
+ %iv = phi i16 [ 0, %entry ], [ %iv.next, %latch ]
+ br i1 %c, label %trap, label %latch
+
+trap:
+ unreachable
+
+second:
+ br i1 false, label %second, label %second
+}
|
|
@llvm/pr-subscribers-llvm-transforms Author: Oscar Priego (Opriego) ChangesFixes #218392.
If Clear a cached This is similar to #215237, which clears stale Added a reduced regression test based on #218392. Testing:
Full diff: https://github.com/llvm/llvm-project/pull/218563.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 63f955538a8d7..97fd016a63d2c 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -8330,6 +8330,12 @@ LoopVectorizeResult LoopVectorizePass::runImpl(Function &F) {
Changed |= CFGChanged |=
simplifyLoop(L, DT, LI, SE, AC, nullptr, false /* PreserveLCSSA */);
+ // Loop simplification may change the CFG before processLoop() lazily requests
+ // BlockFrequencyAnalysis. Clear a cached CycleAnalysis so BFI recomputes it
+ // for the simplified CFG.
+ if (CFGChanged && FAM->getCachedResult<CycleAnalysis>(F))
+ FAM->clearAnalysis<CycleAnalysis>(F);
+
// Build up a worklist of inner-loops to vectorize. This is necessary as
// the act of vectorizing or partially unrolling a loop creates new loops
// and can invalidate iterators across the loops.
diff --git a/llvm/test/Transforms/LoopVectorize/bfi-stale-after-loop-simplify.ll b/llvm/test/Transforms/LoopVectorize/bfi-stale-after-loop-simplify.ll
new file mode 100644
index 0000000000000..1dfe4882f9193
--- /dev/null
+++ b/llvm/test/Transforms/LoopVectorize/bfi-stale-after-loop-simplify.ll
@@ -0,0 +1,25 @@
+; RUN: opt < %s -passes='function(require<cycles>,loop-vectorize)' -disable-output
+
+; LoopVectorize simplifies loops before processing them. If CycleAnalysis was
+; cached before the pass, it must not be reused after loop simplification
+; changes the CFG when BlockFrequencyInfo is requested lazily.
+
+define i16 @f(i1 %c) {
+entry:
+ br label %header
+
+latch:
+ %iv.next = add i16 %iv, 1
+ %done = icmp eq i16 %iv.next, 0
+ br i1 %done, label %second, label %header
+
+header:
+ %iv = phi i16 [ 0, %entry ], [ %iv.next, %latch ]
+ br i1 %c, label %trap, label %latch
+
+trap:
+ unreachable
+
+second:
+ br i1 false, label %second, label %second
+}
|
I confirm that I've read these policies |
| // Loop simplification may change the CFG before processLoop() lazily requests | ||
| // BlockFrequencyAnalysis. Clear a cached CycleAnalysis so BFI recomputes it | ||
| // for the simplified CFG. | ||
| if (CFGChanged && FAM->getCachedResult<CycleAnalysis>(F)) |
There was a problem hiding this comment.
With this, we can probably revert the code from #215237
2 participants
Fixes #218392.
LoopVectorizePass::runImpl()simplifies loops before processing them. Thatinitial
simplifyLoop()phase may change the CFG.If
CycleAnalysiswas already cached by an earlier pass, the cached resultstill describes the pre-simplified CFG. Later,
processLoop()may requestBlockFrequencyAnalysislazily from the cost model. Since BFI now depends onCycleAnalysis, it can consume the stale cycle information and hit:Clear a cached
CycleAnalysisimmediately after the initial loopsimplification when the CFG changed, so a later BFI request recomputes cycle
information for the simplified CFG.
This is similar to #215237, which clears stale
CycleAnalysisaftervectorizing a loop. This change covers the earlier CFG-changing simplification
phase before the first loop is processed.
Added a reduced regression test based on #218392.
Testing:
bfi-stale-after-loop-simplify.ll: PASSbfi-stale-crash.ll: PASSfunction(require<cycles>,loop-vectorize,print<block-freq>): exit 0AI-assisted development tools were used during this contribution; I personally modified, reviewed, and validated the final patch