-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[ScalarEvolution] Handle addrec incoming value in isImpliedViaMerge() #126236
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
The code already guards against values coming from a previous iteration using properlyDominates(). However, addrecs are considered to properly dominate the loop they are defined in. Handle this special case separately, by checking for expressions that have computable loop evolution (this should cover cases like a zext of an addrec as well). I considered changing the definition of properlyDominates() instead, but decided against it. The current definition is useful in other context, e.g. when deciding whether an expression is safe to expand in a given block. Fixes llvm#126012.
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-analysis Author: Nikita Popov (nikic) ChangesThe code already guards against values coming from a previous iteration using properlyDominates(). However, addrecs are considered to properly dominate the loop they are defined in. Handle this special case separately, by checking for expressions that have computable loop evolution (this should cover cases like a zext of an addrec as well). I considered changing the definition of properlyDominates() instead, but decided against it. The current definition is useful in other context, e.g. when deciding whether an expression is safe to expand in a given block. Fixes #126012. Full diff: https://github.com/llvm/llvm-project/pull/126236.diff 2 Files Affected:
diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index f89887118d8d745..46a5c44f4e41a75 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -12400,6 +12400,12 @@ bool ScalarEvolution::isImpliedViaMerge(CmpPredicate Pred, const SCEV *LHS,
// iteration of a loop.
if (!properlyDominates(L, LBB))
return false;
+ // Addrecs are considered to properly dominate their loop, so are missed
+ // by the previous check. Discard any values that have computable
+ // evolution in this loop.
+ if (auto *Loop = LI.getLoopFor(LBB))
+ if (hasComputableLoopEvolution(L, Loop))
+ return false;
if (!ProvedEasily(L, RHS))
return false;
}
diff --git a/llvm/test/Transforms/IndVarSimplify/pr126012.ll b/llvm/test/Transforms/IndVarSimplify/pr126012.ll
index 725ea89b8e65189..5189fe020dd3bfd 100644
--- a/llvm/test/Transforms/IndVarSimplify/pr126012.ll
+++ b/llvm/test/Transforms/IndVarSimplify/pr126012.ll
@@ -1,18 +1,22 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
; RUN: opt -S -passes=indvars < %s | FileCheck %s
-; FIXME: This is a miscompile.
+; Do not infer that %cmp is true. The %indvar3 input of %indvar2 comes from
+; a previous iteration, so we should not compare it to a value from the current
+; iteration.
define i32 @test() {
; CHECK-LABEL: define i32 @test() {
; CHECK-NEXT: [[ENTRY:.*]]:
; CHECK-NEXT: br label %[[FOR_PREHEADER:.*]]
; CHECK: [[FOR_PREHEADER]]:
; CHECK-NEXT: [[INDVAR1:%.*]] = phi i32 [ 0, %[[ENTRY]] ], [ [[PHI:%.*]], %[[FOR_INC:.*]] ]
-; CHECK-NEXT: [[INDVAR3:%.*]] = phi i32 [ 0, %[[ENTRY]] ], [ [[INC:%.*]], %[[FOR_INC]] ]
+; CHECK-NEXT: [[INDVAR2:%.*]] = phi i32 [ 1, %[[ENTRY]] ], [ [[INDVAR3:%.*]], %[[FOR_INC]] ]
+; CHECK-NEXT: [[INDVAR3]] = phi i32 [ 0, %[[ENTRY]] ], [ [[INC:%.*]], %[[FOR_INC]] ]
; CHECK-NEXT: [[COND1:%.*]] = icmp eq i32 [[INDVAR3]], 0
; CHECK-NEXT: br i1 [[COND1]], label %[[FOR_INC]], label %[[FOR_END:.*]]
; CHECK: [[FOR_END]]:
-; CHECK-NEXT: [[EXT:%.*]] = zext i1 true to i32
+; CHECK-NEXT: [[CMP:%.*]] = icmp ugt i32 [[INDVAR2]], 0
+; CHECK-NEXT: [[EXT:%.*]] = zext i1 [[CMP]] to i32
; CHECK-NEXT: br label %[[FOR_INC]]
; CHECK: [[FOR_INC]]:
; CHECK-NEXT: [[PHI]] = phi i32 [ [[EXT]], %[[FOR_END]] ], [ 0, %[[FOR_PREHEADER]] ]
|
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!
/cherry-pick 7aed53e |
Failed to cherry-pick: 7aed53e https://github.com/llvm/llvm-project/actions/runs/13237235524 Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request |
/pull-request #126492 |
…llvm#126236) The code already guards against values coming from a previous iteration using properlyDominates(). However, addrecs are considered to properly dominate the loop they are defined in. Handle this special case separately, by checking for expressions that have computable loop evolution (this should cover cases like a zext of an addrec as well). I considered changing the definition of properlyDominates() instead, but decided against it. The current definition is useful in other context, e.g. when deciding whether an expression is safe to expand in a given block. Fixes llvm#126012. (cherry picked from commit 7aed53e)
…llvm#126236) The code already guards against values coming from a previous iteration using properlyDominates(). However, addrecs are considered to properly dominate the loop they are defined in. Handle this special case separately, by checking for expressions that have computable loop evolution (this should cover cases like a zext of an addrec as well). I considered changing the definition of properlyDominates() instead, but decided against it. The current definition is useful in other context, e.g. when deciding whether an expression is safe to expand in a given block. Fixes llvm#126012.
The code already guards against values coming from a previous iteration using properlyDominates(). However, addrecs are considered to properly dominate the loop they are defined in.
Handle this special case separately, by checking for expressions that have computable loop evolution (this should cover cases like a zext of an addrec as well).
I considered changing the definition of properlyDominates() instead, but decided against it. The current definition is useful in other context, e.g. when deciding whether an expression is safe to expand in a given block.
Fixes #126012.