[SCEV] Fix false-positive recognition of simple recurrences. PR49856

[mkazantsev]

Summary

A value from reachable block may come to a Phi node as its input from
unreachable block. This may confuse matchSimpleRecurrence which
has no access to DomTree and can falsely recognize something as a recurrency
because of this effect, as the attached test shows.

Patch ae7b1e deals with half of this problem, but it only accounts from
the case when an unreachable instruction comes to Phi as an input.

This patch provides a generalization by checking that no Phi block's
predecessor is unreachable (no matter what the input is).

[mkazantsev]

Trivialized test.

[reames]

I posted an alternate fix here: https://reviews.llvm.org/D100004. Let me know what you think. Stylistically, I prefer that one, but yours appears to be correct as well.

Given that, I'm LGTMing this, but asking you not to land unless you spot a problem with the one I linked to. If you do, feel free to land this, and I can work on top of it. I don't want to delay the correctness fix over a debatable style issue.

[mkazantsev]

I can't say if the alternative a correct solution because I don't know if we guarantee to destroy all loops immediately after some block contained in them has become urneachable. If not, then the alternative solution also contains this bug. If yes, this is not obvious and I'm not sure where it is asserted. We can go with it if we have the answers to these questions. I don't see any problems with having a loop in fully unreachable code, or having extra unreachable predecessors to loop blocks.

llvm/lib/Analysis/ScalarEvolution.cpp

getRangeForUnknownRecurrence(const SCEVUnknown *U) {
   // the one used by AddRec (and thus most of this file).  Step is allowed to
   // be arbitrarily loop varying here, where AddRec allows only loop invariant
   // and other addrecs in the same loop (for non-affine addrecs).  The code
   // below intentionally handles the case where step is not loop invariant.
   auto *P = dyn_cast<PHINode>(U->getValue());
   if (!P)
     return CR;
 
+  // Make sure that no Phi input comes from an unreachable block. Otherwise,
+  // even the values that are not available in these blocks may come from them,
+  // and this leads to false-positive recurrence test.
+  for (auto *Pred : predecessors(P->getParent()))
+    if (!DT.isReachableFromEntry(Pred))
+      return CR;
+
   BinaryOperator *BO;
   Value *Start, *Step;
   if (!matchSimpleRecurrence(P, BO, Start, Step))
     return CR;
 
-  if (!DT.isReachableFromEntry(P->getParent()) ||
-      !DT.isReachableFromEntry(BO->getParent()))
-    // If either is in unreachable code, dominance collapses and none of our
-    // expected post conditions about loops hold.
-    return CR;
-
   // If we found a recurrence in reachable code, we must be in a loop. Note
   // that BO might be in some subloop of L, and that's completely okay.
   auto *L = LI.getLoopFor(P->getParent());
   assert(L && L->getHeader() == P->getParent());
   if (!L->contains(BO->getParent()))
     // NOTE: This bailout should be an assert instead.  However, asserting
     // the condition here exposes a case where LoopFusion is querying SCEV
     // with malformed loop information during the midst of the transform.

llvm/test/Analysis/ScalarEvolution/pr49856.ll

+; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py
+; RUN: opt < %s -analyze -enable-new-pm=0 -scalar-evolution | FileCheck %s
+; RUN: opt < %s -disable-output "-passes=print<scalar-evolution>" 2>&1 | FileCheck %s
+
+define void @test() {
+; CHECK-LABEL: 'test'
+; CHECK-NEXT:  Classifying expressions for: @test
+; CHECK-NEXT:    %tmp = phi i32 [ 2, %bb ], [ %tmp2, %bb3 ]
+; CHECK-NEXT:    --> %tmp U: [1,-2147483648) S: [0,-2147483648)
+; CHECK-NEXT:    %tmp2 = add nuw nsw i32 %tmp, 1
+; CHECK-NEXT:    --> (1 + %tmp)<nuw> U: [1,-2147483647) S: [1,-2147483647)
+; CHECK-NEXT:  Determining loop execution counts for: @test
+;
+bb:
+  br label %bb1
+
+bb1:                                              ; preds = %bb3, %bb
+  %tmp = phi i32 [ 2, %bb ], [ %tmp2, %bb3 ]
+  %tmp2 = add nuw nsw i32 %tmp, 1
+  ret void
+
+bb3:                                              ; No predecessors!
+  br label %bb1
+}