[turbofan] Fix bug in Typer::TypeInductionVariablePhi, again

GeorgNeis · Commit Bot · commit 68099bffaca0 · 2020-02-19T16:40:45.000Z
Regrettably the previous fix was flawed because a zero increment can change the type of the induction variable. Bug: chromium:1051017 Change-Id: I2d7aeffb2065e739445118a2d0c5f7732eecdcbb Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2064222 Reviewed-by: Michael Stanton <mvstanton@chromium.org> Reviewed-by: Tobias Tebbi <tebbi@chromium.org> Commit-Queue: Georg Neis <neis@chromium.org> Cr-Commit-Position: refs/heads/master@{#66345}
diff --git a/src/compiler/typer.cc b/src/compiler/typer.cc
@@ -850,26 +850,21 @@ Type Typer::Visitor::TypeInductionVariablePhi(Node* node) {
   Type initial_type = Operand(node, 0);
   Type increment_type = Operand(node, 2);
 
-  // If we do not have enough type information for the initial value or
-  // the increment, just return the initial value's type.
+  // Fallback to normal phi typing in a variety of cases: when the induction
+  // variable is not initially of type Integer (we want to work with ranges
+  // below), when the increment is zero (in that case, phi typing will generally
+  // yield a better type), and when the induction variable can become NaN
+  // (through addition/subtraction of opposing infinities).
   if (initial_type.IsNone() ||
-      increment_type.Is(typer_->cache_->kSingletonZero)) {
-    return initial_type;
-  }
-
-  // We only handle integer induction variables (otherwise ranges do not apply
-  // and we cannot do anything). Moreover, we don't support infinities in
-  // {increment_type} because the induction variable can become NaN through
-  // addition/subtraction of opposing infinities.
-  if (!initial_type.Is(typer_->cache_->kInteger) ||
+      increment_type.Is(typer_->cache_->kSingletonZero) ||
+      !initial_type.Is(typer_->cache_->kInteger) ||
       !increment_type.Is(typer_->cache_->kInteger) ||
       increment_type.Min() == -V8_INFINITY ||
       increment_type.Max() == +V8_INFINITY) {
-    // Fallback to normal phi typing, but ensure monotonicity.
-    // (Unfortunately, without baking in the previous type, monotonicity might
-    // be violated because we might not yet have retyped the incrementing
-    // operation even though the increment's type might been already reflected
-    // in the induction variable phi.)
+    // Unfortunately, without baking in the previous type, monotonicity might be
+    // violated because we might not yet have retyped the incrementing operation
+    // even though the increment's type might been already reflected in the
+    // induction variable phi.
     Type type = NodeProperties::IsTyped(node) ? NodeProperties::GetType(node)
                                               : Type::None();
     for (int i = 0; i < arity; ++i) {
diff --git a/test/mjsunit/compiler/regress-1051017.js b/test/mjsunit/compiler/regress-1051017.js
@@ -32,3 +32,17 @@ assertEquals(NaN, foo2());
 assertEquals(NaN, foo2());
 %OptimizeFunctionOnNextCall(foo2);
 assertEquals(NaN, foo2());
+
+
+function foo3(b) {
+  var k = 0;
+  let str = b ? "42" : "0";
+  for (var i = str; i < 1 && k++ < 1; i -= 0) { }
+  return i;
+}
+
+%PrepareFunctionForOptimization(foo3);
+assertEquals(0, foo3());
+assertEquals(0, foo3());
+%OptimizeFunctionOnNextCall(foo3);
+assertEquals(0, foo3());
