[SCEV] Do not allow refinement in the rewriting of BEValue #117152

dtcxzyw · 2024-11-21T12:42:29Z

See the following case:

; bin/opt -passes="print<scalar-evolution>" test.ll --disable-output
define i32 @widget() {
b:
  br label %b1

b1:                                              ; preds = %b5, %b
  %phi = phi i32 [ 0, %b ], [ %udiv6, %b5 ]
  %phi2 = phi i32 [ 1, %b ], [ %add, %b5 ]
  %icmp = icmp eq i32 %phi, 0
  br i1 %icmp, label %b3, label %b8

b3:                                              ; preds = %b1
  %udiv = udiv i32 10, %phi2
  %urem = urem i32 %udiv, 10
  %icmp4 = icmp eq i32 %urem, 0
  br i1 %icmp4, label %b7, label %b5

b5:                                              ; preds = %b3
  %udiv6 = udiv i32 %phi2, 0
  %add = add i32 %phi2, 1
  br label %b1

b7:                                              ; preds = %b3
  ret i32 5

b8:                                              ; preds = %b1
  ret i32 7
}

%phi2 = phi i32 [ 1, %b ], [ %add, %b5 ] -->  {1,+,1}<nuw><nsw><%b1>
%udiv6 = udiv i32 %phi2, 0 --> ({1,+,1}<nuw><nsw><%b1> /u 0)
%phi = phi i32 [ 0, %b ], [ %udiv6, %b5 ] --> ({0,+,1}<nuw><nsw><%b1> /u 0)

ScalarEvolution::createAddRecFromPHI gives a wrong SCEV result for %phi:

llvm-project/llvm/lib/Analysis/ScalarEvolution.cpp

Lines 5926 to 5950 in d7d6fb1

    
           } else { 
        
             // Otherwise, this could be a loop like this: 
        
             //     i = 0;  for (j = 1; ..; ++j) { ....  i = j; } 
        
             // In this case, j = {1,+,1}  and BEValue is j. 
        
             // Because the other in-value of i (0) fits the evolution of BEValue 
        
             // i really is an addrec evolution. 
        
             // 
        
             // We can generalize this saying that i is the shifted value of BEValue 
        
             // by one iteration: 
        
             //   PHI(f(0), f({1,+,1})) --> f({0,+,1}) 
        
             const SCEV *Shifted = SCEVShiftRewriter::rewrite(BEValue, L, *this); 
        
             const SCEV *Start = SCEVInitRewriter::rewrite(Shifted, L, *this, false); 
        
             if (Shifted != getCouldNotCompute() && 
        
                 Start != getCouldNotCompute()) { 
        
               const SCEV *StartVal = getSCEV(StartValueV); 
        
               if (Start == StartVal) { 
        
                 // Okay, for the entire analysis of this edge we assumed the PHI 
        
                 // to be symbolic.  We now need to go back and purge all of the 
        
                 // entries for the scalars that use the symbolic expression. 
        
                 forgetMemoizedResults(SymbolicName); 
        
                 insertValueToMap(PN, Shifted); 
        
                 return Shifted; 
        
               } 
        
             } 
        
           }

It converts phi(0, ({1,+,1}<nuw><nsw><%b1> /u 0)) into phi(0 / 0, ({1,+,1}<nuw><nsw><%b1> /u 0)). Then it simplifies the expr into {0,+,1}<nuw><nsw><%b1> /u 0.

As we did in acd700a, this patch disallows udiv simplification if we cannot prove that the denominator is a well-defined non-zero value.

Fixes #117133.

llvmbot · 2024-11-21T12:43:12Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: Yingwei Zheng (dtcxzyw)

Changes

See the following case:

; bin/opt -passes="print&lt;scalar-evolution&gt;" test.ll --disable-output
define i32 @<!-- -->widget() {
b:
  br label %b1

b1:                                              ; preds = %b5, %b
  %phi = phi i32 [ 0, %b ], [ %udiv6, %b5 ]
  %phi2 = phi i32 [ 1, %b ], [ %add, %b5 ]
  %icmp = icmp eq i32 %phi, 0
  br i1 %icmp, label %b3, label %b8

b3:                                              ; preds = %b1
  %udiv = udiv i32 10, %phi2
  %urem = urem i32 %udiv, 10
  %icmp4 = icmp eq i32 %urem, 0
  br i1 %icmp4, label %b7, label %b5

b5:                                              ; preds = %b3
  %udiv6 = udiv i32 %phi2, 0
  %add = add i32 %phi2, 1
  br label %b1

b7:                                              ; preds = %b3
  ret i32 5

b8:                                              ; preds = %b1
  ret i32 7
}

%phi2 = phi i32 [ 1, %b ], [ %add, %b5 ] --&gt;  {1,+,1}&lt;nuw&gt;&lt;nsw&gt;&lt;%b1&gt;
%udiv6 = udiv i32 %phi2, 0 --&gt; ({1,+,1}&lt;nuw&gt;&lt;nsw&gt;&lt;%b1&gt; /u 0)
%phi = phi i32 [ 0, %b ], [ %udiv6, %b5 ] --&gt; ({0,+,1}&lt;nuw&gt;&lt;nsw&gt;&lt;%b1&gt; /u 0)

ScalarEvolution::createAddRecFromPHI gives a wrong SCEV result for %phi:

llvm-project/llvm/lib/Analysis/ScalarEvolution.cpp

Lines 5926 to 5950 in d7d6fb1

    
           } else { 
        
             // Otherwise, this could be a loop like this: 
        
             //     i = 0;  for (j = 1; ..; ++j) { ....  i = j; } 
        
             // In this case, j = {1,+,1}  and BEValue is j. 
        
             // Because the other in-value of i (0) fits the evolution of BEValue 
        
             // i really is an addrec evolution. 
        
             // 
        
             // We can generalize this saying that i is the shifted value of BEValue 
        
             // by one iteration: 
        
             //   PHI(f(0), f({1,+,1})) --> f({0,+,1}) 
        
             const SCEV *Shifted = SCEVShiftRewriter::rewrite(BEValue, L, *this); 
        
             const SCEV *Start = SCEVInitRewriter::rewrite(Shifted, L, *this, false); 
        
             if (Shifted != getCouldNotCompute() && 
        
                 Start != getCouldNotCompute()) { 
        
               const SCEV *StartVal = getSCEV(StartValueV); 
        
               if (Start == StartVal) { 
        
                 // Okay, for the entire analysis of this edge we assumed the PHI 
        
                 // to be symbolic.  We now need to go back and purge all of the 
        
                 // entries for the scalars that use the symbolic expression. 
        
                 forgetMemoizedResults(SymbolicName); 
        
                 insertValueToMap(PN, Shifted); 
        
                 return Shifted; 
        
               } 
        
             } 
        
           }

It converts phi(0, ({1,+,1}<nuw><nsw><%b1> /u 0)) into phi(0 / 0, ({1,+,1}<nuw><nsw><%b1> /u 0)). Then it simplifies the expr into {0,+,1}<nuw><nsw><%b1> /u 0.

As we did in acd700a, this patch disallows udiv simplification if we cannot prove that the denominator is a well-defined non-zero value.

Fixes #117133.

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

5 Files Affected:

(modified) llvm/lib/Analysis/ScalarEvolution.cpp (+7-2)
(modified) llvm/test/Analysis/ScalarEvolution/fold.ll (+1-1)
(added) llvm/test/Analysis/ScalarEvolution/pr117133.ll (+53)
(modified) llvm/test/Analysis/ScalarEvolution/udiv-of-x-xsmaxone-fold.ll (+2-2)
(added) llvm/test/Transforms/IndVarSimplify/pr117133.ll (+45)

diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index 376f260846bbaa..4d0fcfef78f1c2 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -3422,9 +3422,14 @@ const SCEV *ScalarEvolution::getUDivExpr(const SCEV *LHS,
   if (const SCEV *S = UniqueSCEVs.FindNodeOrInsertPos(ID, IP))
     return S;
 
+  // If the denominator is zero, the udiv will trap.
+  auto IsValidDenominator = [&] {
+    return isGuaranteedNotToBePoison(RHS) && isKnownNonZero(RHS);
+  };
+
   // 0 udiv Y == 0
   if (const SCEVConstant *LHSC = dyn_cast<SCEVConstant>(LHS))
-    if (LHSC->getValue()->isZero())
+    if (LHSC->getValue()->isZero() && IsValidDenominator())
       return LHS;
 
   if (const SCEVConstant *RHSC = dyn_cast<SCEVConstant>(RHS)) {
@@ -3560,7 +3565,7 @@ const SCEV *ScalarEvolution::getUDivExpr(const SCEV *LHS,
         if (MME && MME->getNumOperands() == 2 &&
             isa<SCEVConstant>(MME->getOperand(0)) &&
             cast<SCEVConstant>(MME->getOperand(0))->getAPInt() == -NegC &&
-            MME->getOperand(1) == RHS)
+            MME->getOperand(1) == RHS && IsValidDenominator())
           return getZero(LHS->getType());
       }
     }
diff --git a/llvm/test/Analysis/ScalarEvolution/fold.ll b/llvm/test/Analysis/ScalarEvolution/fold.ll
index 670523ca1bb5b9..5fde3b18da9fe9 100644
--- a/llvm/test/Analysis/ScalarEvolution/fold.ll
+++ b/llvm/test/Analysis/ScalarEvolution/fold.ll
@@ -214,7 +214,7 @@ define i64 @test10(i64 %a, i64 %b) {
   ret i64 %t2
 }
 
-define i64 @test11(i64 %a) {
+define i64 @test11(i64 noundef range(i64 1, 0) %a) {
 ; CHECK-LABEL: 'test11'
 ; CHECK-NEXT:  Classifying expressions for: @test11
 ; CHECK-NEXT:    %t0 = udiv i64 0, %a
diff --git a/llvm/test/Analysis/ScalarEvolution/pr117133.ll b/llvm/test/Analysis/ScalarEvolution/pr117133.ll
new file mode 100644
index 00000000000000..75f15055ce3d94
--- /dev/null
+++ b/llvm/test/Analysis/ScalarEvolution/pr117133.ll
@@ -0,0 +1,53 @@
+; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -disable-output "-passes=print<scalar-evolution>" < %s 2>&1 | FileCheck %s
+
+define i32 @widget() {
+; CHECK-LABEL: 'widget'
+; CHECK-NEXT:  Classifying expressions for: @widget
+; CHECK-NEXT:    %phi = phi i32 [ 0, %b ], [ %udiv6, %b5 ]
+; CHECK-NEXT:    --> %phi U: [0,1) S: [0,1) Exits: <<Unknown>> LoopDispositions: { %b1: Variant }
+; CHECK-NEXT:    %phi2 = phi i32 [ 1, %b ], [ %add, %b5 ]
+; CHECK-NEXT:    --> {1,+,1}<nuw><nsw><%b1> U: [1,2) S: [1,2) Exits: <<Unknown>> LoopDispositions: { %b1: Computable }
+; CHECK-NEXT:    %udiv = udiv i32 10, %phi2
+; CHECK-NEXT:    --> (10 /u {1,+,1}<nuw><nsw><%b1>) U: [10,11) S: [10,11) Exits: <<Unknown>> LoopDispositions: { %b1: Computable }
+; CHECK-NEXT:    %urem = urem i32 %udiv, 10
+; CHECK-NEXT:    --> ((-10 * ((10 /u {1,+,1}<nuw><nsw><%b1>) /u 10))<nuw><nsw> + (10 /u {1,+,1}<nuw><nsw><%b1>)) U: [0,1) S: [0,1) Exits: <<Unknown>> LoopDispositions: { %b1: Computable }
+; CHECK-NEXT:    %udiv6 = udiv i32 %phi2, 0
+; CHECK-NEXT:    --> ({1,+,1}<nuw><nsw><%b1> /u 0) U: empty-set S: empty-set Exits: <<Unknown>> LoopDispositions: { %b1: Computable }
+; CHECK-NEXT:    %add = add i32 %phi2, 1
+; CHECK-NEXT:    --> {2,+,1}<nuw><nsw><%b1> U: [2,3) S: [2,3) Exits: <<Unknown>> LoopDispositions: { %b1: Computable }
+; CHECK-NEXT:  Determining loop execution counts for: @widget
+; CHECK-NEXT:  Loop %b1: <multiple exits> Unpredictable backedge-taken count.
+; CHECK-NEXT:    exit count for b1: ***COULDNOTCOMPUTE***
+; CHECK-NEXT:    exit count for b3: i32 0
+; CHECK-NEXT:  Loop %b1: constant max backedge-taken count is i32 0
+; CHECK-NEXT:  Loop %b1: symbolic max backedge-taken count is i32 0
+; CHECK-NEXT:    symbolic max exit count for b1: ***COULDNOTCOMPUTE***
+; CHECK-NEXT:    symbolic max exit count for b3: i32 0
+;
+b:
+  br label %b1
+
+b1:                                              ; preds = %b5, %b
+  %phi = phi i32 [ 0, %b ], [ %udiv6, %b5 ]
+  %phi2 = phi i32 [ 1, %b ], [ %add, %b5 ]
+  %icmp = icmp eq i32 %phi, 0
+  br i1 %icmp, label %b3, label %b8
+
+b3:                                              ; preds = %b1
+  %udiv = udiv i32 10, %phi2
+  %urem = urem i32 %udiv, 10
+  %icmp4 = icmp eq i32 %urem, 0
+  br i1 %icmp4, label %b7, label %b5
+
+b5:                                              ; preds = %b3
+  %udiv6 = udiv i32 %phi2, 0
+  %add = add i32 %phi2, 1
+  br label %b1
+
+b7:                                              ; preds = %b3
+  ret i32 5
+
+b8:                                              ; preds = %b1
+  ret i32 7
+}
diff --git a/llvm/test/Analysis/ScalarEvolution/udiv-of-x-xsmaxone-fold.ll b/llvm/test/Analysis/ScalarEvolution/udiv-of-x-xsmaxone-fold.ll
index 9405c0f726ac7f..5694a505ccb316 100644
--- a/llvm/test/Analysis/ScalarEvolution/udiv-of-x-xsmaxone-fold.ll
+++ b/llvm/test/Analysis/ScalarEvolution/udiv-of-x-xsmaxone-fold.ll
@@ -1,7 +1,7 @@
 ; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py
 ; RUN: opt -disable-output -passes="print<scalar-evolution>" < %s 2>&1 | FileCheck %s
 
-define i32 @test_expr_with_constant_1(i32 %x) {
+define i32 @test_expr_with_constant_1(i32 noundef range(i32 1, 0) %x) {
 ; CHECK-LABEL: 'test_expr_with_constant_1'
 ; CHECK-NEXT:  Classifying expressions for: @test_expr_with_constant_1
 ; CHECK-NEXT:    %smax = tail call i32 @llvm.smax.i32(i32 %x, i32 1)
@@ -20,7 +20,7 @@ entry:
 }
 
 ; Non-1 constant: (-2 + (2 smax %x)) /u %x
-define i32 @test_expr_with_constant_2(i32 %x) {
+define i32 @test_expr_with_constant_2(i32 noundef range(i32 1, 0) %x) {
 ; CHECK-LABEL: 'test_expr_with_constant_2'
 ; CHECK-NEXT:  Classifying expressions for: @test_expr_with_constant_2
 ; CHECK-NEXT:    %smax = tail call i32 @llvm.smax.i32(i32 %x, i32 2)
diff --git a/llvm/test/Transforms/IndVarSimplify/pr117133.ll b/llvm/test/Transforms/IndVarSimplify/pr117133.ll
new file mode 100644
index 00000000000000..31545ecba94cdb
--- /dev/null
+++ b/llvm/test/Transforms/IndVarSimplify/pr117133.ll
@@ -0,0 +1,45 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=indvars < %s | FileCheck %s
+
+
+define i32 @widget() {
+; CHECK-LABEL: define i32 @widget() {
+; CHECK-NEXT:  [[B:.*:]]
+; CHECK-NEXT:    br label %[[B1:.*]]
+; CHECK:       [[B1]]:
+; CHECK-NEXT:    br i1 true, label %[[B3:.*]], label %[[B8:.*]]
+; CHECK:       [[B3]]:
+; CHECK-NEXT:    br i1 true, label %[[B7:.*]], label %[[B5:.*]]
+; CHECK:       [[B5]]:
+; CHECK-NEXT:    br label %[[B1]]
+; CHECK:       [[B7]]:
+; CHECK-NEXT:    ret i32 5
+; CHECK:       [[B8]]:
+; CHECK-NEXT:    ret i32 7
+;
+b:
+  br label %b1
+
+b1:
+  %phi = phi i32 [ 0, %b ], [ %udiv6, %b5 ]
+  %phi2 = phi i32 [ 1, %b ], [ %add, %b5 ]
+  %icmp = icmp eq i32 %phi, 0
+  br i1 %icmp, label %b3, label %b8
+
+b3:
+  %udiv = udiv i32 10, %phi2
+  %urem = urem i32 %udiv, 10
+  %icmp4 = icmp eq i32 %urem, 0
+  br i1 %icmp4, label %b7, label %b5
+
+b5:
+  %udiv6 = udiv i32 %phi2, 0
+  %add = add i32 %phi2, 1
+  br label %b1
+
+b7:
+  ret i32 5
+
+b8:
+  ret i32 7
+}

llvm/lib/Analysis/ScalarEvolution.cpp

nikic

I don't think that this approach is correct. If I understand correctly, you're basically trying to add the invariant that simplification of SCEV expressions must preserve undefined behavior. But to achieve that, this would be only a very partial implementation, e.g. we could also lose it through %div * 0 or %div - %div or any number of other ways.

I think the problem here is rather in that particular addrec construction code. We basically have a derefinement issue here, similar to what we see in select simplification.

We could hit similar derefinement issues with poison rather than UB, probably some example involving umin_seq(%maybe_poison, 0). And with @fhahn's patches for per-use nowrap flags, this would become a more significant problem.

So I think what we need to guard against is derefinement when replacing the start value.

dtcxzyw · 2024-11-21T13:45:15Z

If I understand correctly, you're basically trying to add the invariant that simplification of SCEV expressions must preserve undefined behavior.

Yeah. For this case, ScalarEvolution::createAddRecFromPHI only checks initval == refine(f(0)). It doesn't imply initval == f(0).

I think the problem here is rather in that particular addrec construction code. We basically have a derefinement issue here, similar to what we see in select simplification.

Do we need an AllowRefinement argument for SCEV constructors and disallow refinement in these rewriters?

nikic · 2024-11-21T13:48:05Z

Do we need an AllowRefinement argument for SCEV constructors and disallow refinement in these rewriters?

Assuming this special case isn't particularly important, can we get away with just checking that the SCEV doesn't contain UB / poison?

llvm/lib/Analysis/ScalarEvolution.cpp

Nirhar · 2024-11-28T15:28:16Z

ping @dtcxzyw !

llvm/lib/Analysis/ScalarEvolution.cpp

llvm/test/Analysis/ScalarEvolution/fold.ll

llvm/test/Analysis/ScalarEvolution/udiv-of-x-xsmaxone-fold.ll

nikic · 2024-11-29T16:51:12Z

llvm/test/Analysis/ScalarEvolution/pr117133.ll

It would be great to construct a test case where poison is problematic.

dtcxzyw · 2024-11-30T06:19:22Z

llvm/test/Analysis/ScalarEvolution/pr117133.ll

+}
+
+; Don't fold %indvar2 into (zext {0,+,1}) * %a
+define i64 @test_poisonous(i64 %a, i32 %n) {


Before:

Printing analysis 'Scalar Evolution Analysis' for function 'test': Classifying expressions for: @test %indvar1 = phi i32 [ 0, %entry ], [ %indvar1.next, %loop.body ] --> {0,+,1}<%loop.body> U: full-set S: full-set Exits: <<Unknown>> LoopDispositions: { %loop.body: Computable } %indvar2 = phi i64 [ 0, %entry ], [ %mul, %loop.body ] --> ((zext i32 {0,+,1}<%loop.body> to i64) * %a) U: full-set S: full-set Exits: <<Unknown>> LoopDispositions: { %loop.body: Computable } %indvar1.next = add i32 %indvar1, 1 --> {1,+,1}<%loop.body> U: full-set S: full-set Exits: <<Unknown>> LoopDispositions: { %loop.body: Computable } %ext = zext i32 %indvar1.next to i64 --> (zext i32 {1,+,1}<%loop.body> to i64) U: [0,4294967296) S: [0,4294967296) Exits: <<Unknown>> LoopDispositions: { %loop.body: Computable } %mul = mul i64 %ext, %a --> ((zext i32 {1,+,1}<%loop.body> to i64) * %a) U: full-set S: full-set Exits: <<Unknown>> LoopDispositions: { %loop.body: Computable }

nikic

LGTM

dtcxzyw · 2024-12-01T12:11:44Z

/cherry-pick f7ef072

llvmbot · 2024-12-01T12:16:59Z

Failed to cherry-pick: f7ef072

https://github.com/llvm/llvm-project/actions/runs/12105303593

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

…lue (llvm#117152)

This is a followup to llvm#117152. That patch introduced a check for UB/poison on BEValue. However, the SCEV we're actually going to use is Shifted. In some cases, it's possible for Shifted to contain UB, while BEValue doesn't. In the test case the values are: BEValue: (-1 * (zext i8 (-83 + ((-83 /u {1,+,1}<%loop>) * {-1,+,-1}<%loop>)) to i32))<nuw><nsw> Shifted: (-173 + (-1 * (zext i8 ((-83 /u {0,+,1}<%loop>) * {0,+,-1}<%loop>) to i32))<nuw><nsw>)<nuw><nsw> Fixes llvm#123550.

This is a followup to #117152. That patch introduced a check for UB/poison on BEValue. However, the SCEV we're actually going to use is Shifted. In some cases, it's possible for Shifted to contain UB, while BEValue doesn't. In the test case the values are: BEValue: (-1 * (zext i8 (-83 + ((-83 /u {1,+,1}<%loop>) * {-1,+,-1}<%loop>)) to i32))<nuw><nsw> Shifted: (-173 + (-1 * (zext i8 ((-83 /u {0,+,1}<%loop>) * {0,+,-1}<%loop>) to i32))<nuw><nsw>)<nuw><nsw> Fixes #123550.

This is a followup to llvm#117152. That patch introduced a check for UB/poison on BEValue. However, the SCEV we're actually going to use is Shifted. In some cases, it's possible for Shifted to contain UB, while BEValue doesn't. In the test case the values are: BEValue: (-1 * (zext i8 (-83 + ((-83 /u {1,+,1}<%loop>) * {-1,+,-1}<%loop>)) to i32))<nuw><nsw> Shifted: (-173 + (-1 * (zext i8 ((-83 /u {0,+,1}<%loop>) * {0,+,-1}<%loop>) to i32))<nuw><nsw>)<nuw><nsw> Fixes llvm#123550. (cherry picked from commit 07efe2c)

[SCEV] Add pre-commit tests. NFC.

8aecde3

dtcxzyw requested a review from fhahn November 21, 2024 12:42

dtcxzyw requested a review from nikic as a code owner November 21, 2024 12:42

llvmbot added llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms labels Nov 21, 2024

dtcxzyw mentioned this pull request Nov 21, 2024

Task submission dtcxzyw/llvm-opt-benchmark#1312

Open

dtcxzyw mentioned this pull request Nov 21, 2024

pre-commit: PR117152 dtcxzyw/llvm-opt-benchmark#1737

Closed

fhahn reviewed Nov 21, 2024

View reviewed changes

llvm/lib/Analysis/ScalarEvolution.cpp Outdated Show resolved Hide resolved

llvm/lib/Analysis/ScalarEvolution.cpp Outdated Show resolved Hide resolved

nikic requested changes Nov 21, 2024

View reviewed changes

[SCEV] Do not allow refinement in the rewriting of BEValue

5ba4943

dtcxzyw force-pushed the fix-pr117133 branch from db15881 to 5ba4943 Compare November 21, 2024 15:07

dtcxzyw changed the title ~~[SCEV] Add non-poison/non-zero checks on denominators~~ [SCEV] Do not allow refinement in the rewriting of BEValue Nov 21, 2024

dtcxzyw commented Nov 21, 2024

View reviewed changes

llvm/lib/Analysis/ScalarEvolution.cpp Show resolved Hide resolved

Remove newline

137ff32

[SCEV] Add poison check.

b16665e

nikic reviewed Nov 29, 2024

View reviewed changes

llvm/lib/Analysis/ScalarEvolution.cpp Outdated Show resolved Hide resolved

[SCEV] Use impliesPoison

47c6126

nikic reviewed Nov 29, 2024

View reviewed changes

dtcxzyw added 2 commits November 30, 2024 13:55

[SCEV] Drop test changes

975bc07

[SCEV] Add a test. NFC.

afc42aa

dtcxzyw commented Nov 30, 2024

View reviewed changes

nikic approved these changes Dec 1, 2024

View reviewed changes

dtcxzyw merged commit f7ef072 into llvm:main Dec 1, 2024
8 checks passed

dtcxzyw deleted the fix-pr117133 branch December 1, 2024 12:11

dtcxzyw added this to the LLVM 19.X Release milestone Dec 1, 2024

llvmbot added the release:cherry-pick-failed label Dec 1, 2024

dtcxzyw self-assigned this Dec 1, 2024

dtcxzyw added a commit to dtcxzyw/llvm-project that referenced this pull request Dec 1, 2024

release/19.x: [SCEV] Do not allow refinement in the rewriting of BEVa…

6f08a0f

…lue (llvm#117152)

nikic mentioned this pull request Jan 24, 2025

[SCEV] Check correct value for UB #124302

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SCEV] Do not allow refinement in the rewriting of BEValue #117152

[SCEV] Do not allow refinement in the rewriting of BEValue #117152

Uh oh!

dtcxzyw commented Nov 21, 2024

Uh oh!

llvmbot commented Nov 21, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

nikic left a comment

Uh oh!

dtcxzyw commented Nov 21, 2024

Uh oh!

nikic commented Nov 21, 2024

Uh oh!

Uh oh!

Nirhar commented Nov 28, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikic Nov 29, 2024

Uh oh!

dtcxzyw Nov 30, 2024

Uh oh!

nikic left a comment

Uh oh!

Uh oh!

dtcxzyw commented Dec 1, 2024

Uh oh!

llvmbot commented Dec 1, 2024

Uh oh!

Uh oh!

	} else {
	// Otherwise, this could be a loop like this:
	// i = 0; for (j = 1; ..; ++j) { .... i = j; }
	// In this case, j = {1,+,1} and BEValue is j.
	// Because the other in-value of i (0) fits the evolution of BEValue
	// i really is an addrec evolution.
	//
	// We can generalize this saying that i is the shifted value of BEValue
	// by one iteration:
	// PHI(f(0), f({1,+,1})) --> f({0,+,1})
	const SCEV Shifted = SCEVShiftRewriter::rewrite(BEValue, L, this);
	const SCEV Start = SCEVInitRewriter::rewrite(Shifted, L, this, false);
	if (Shifted != getCouldNotCompute() &&
	Start != getCouldNotCompute()) {
	const SCEV *StartVal = getSCEV(StartValueV);
	if (Start == StartVal) {
	// Okay, for the entire analysis of this edge we assumed the PHI
	// to be symbolic. We now need to go back and purge all of the
	// entries for the scalars that use the symbolic expression.
	forgetMemoizedResults(SymbolicName);
	insertValueToMap(PN, Shifted);
	return Shifted;
	}
	}
	}

[SCEV] Do not allow refinement in the rewriting of BEValue #117152

[SCEV] Do not allow refinement in the rewriting of BEValue #117152

Uh oh!

Conversation

dtcxzyw commented Nov 21, 2024

Uh oh!

llvmbot commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

dtcxzyw commented Nov 21, 2024

Uh oh!

nikic commented Nov 21, 2024

Uh oh!

Uh oh!

Nirhar commented Nov 28, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikic Nov 29, 2024

Choose a reason for hiding this comment

Uh oh!

dtcxzyw Nov 30, 2024

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dtcxzyw commented Dec 1, 2024

Uh oh!

llvmbot commented Dec 1, 2024

Uh oh!

Uh oh!

llvmbot commented Nov 21, 2024 •

edited

Loading