Skip to content

[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

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Feb 7, 2025

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.

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.
@nikic nikic requested review from fhahn, preames and dtcxzyw February 7, 2025 11:58
@llvmbot llvmbot added llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms labels Feb 7, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 7, 2025

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: Nikita Popov (nikic)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/126236.diff

2 Files Affected:

  • (modified) llvm/lib/Analysis/ScalarEvolution.cpp (+6)
  • (modified) llvm/test/Transforms/IndVarSimplify/pr126012.ll (+7-3)
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]] ]

Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@nikic nikic merged commit 7aed53e into llvm:main Feb 10, 2025
11 checks passed
@nikic nikic deleted the scev-implied-merge-fix branch February 10, 2025 09:07
@nikic nikic added this to the LLVM 20.X Release milestone Feb 10, 2025
@nikic
Copy link
Contributor Author

nikic commented Feb 10, 2025

/cherry-pick 7aed53e

@llvmbot
Copy link
Member

llvmbot commented Feb 10, 2025

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

@nikic
Copy link
Contributor Author

nikic commented Feb 10, 2025

/cherry-pick ae08969 7aed53e

@llvmbot
Copy link
Member

llvmbot commented Feb 10, 2025

/pull-request #126492

swift-ci pushed a commit to swiftlang/llvm-project that referenced this pull request Feb 11, 2025
…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)
Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms
Projects
Development

Successfully merging this pull request may close these issues.

[IndVarSimplify] Miscompilation at O2
4 participants