[turbofan] Eliminate redundant Smi checks around array accesses.

As identified in the web-tooling-benchmark, there are specific code
patterns involving array indexed property accesses and subsequent
comparisons of those indices that lead to repeated Smi checks in the
optimized code, which in turn leads to high register pressure and
generally bad register allocation. An example of this pattern is
code like this:

```js
function f(a, n) {
  const i = a[n];
  if (n >= 1) return i;
}
```

The `a[n]` property access introduces a CheckBounds on `n`, which
later lowers to a `CheckedTaggedToInt32[dont-check-minus-zero]`,
however the `n >= 1` comparison has collected `SignedSmall` feedback
and so it introduces a `CheckedTaggedToTaggedSigned` operation. This
second Smi check is redundant and cannot easily be combined with the
earlier tagged->int32 conversion, since that also deals with heap
numbers and even truncates -0 to 0.

So we teach the RedundancyElimination to look at the inputs of these
speculative number comparisons and if there's a leading bounds check
on either of these inputs, we change the input to the result of the
bounds check. This avoids the redundant Smi checks later and generally
allows the SimplifiedLowering to do a significantly better job on the
number comparisons. We only do this in case of SignedSmall feedback
and only for inputs that are not already known to be in UnsignedSmall
range, to avoid doing too many (unnecessary) expensive lookups during
RedundancyElimination.

All of this is safe despite the fact that CheckBounds truncates -0
to 0, since the regular number comparisons in JavaScript identify
0 and -0 (unlike Object.is()). This also adds appropriate tests,
especially for the interesting cases where -0 is used only after
the code was optimized.

Bug: v8:6936, v8:7094
Change-Id: Ie37114fb6192e941ae1a4f0bfe00e9c0a8305c07
Reviewed-on: https://chromium-review.googlesource.com/c/1246181Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56428}

src/compiler/redundancy-elimination.cc

@@ -36,6 +36,10 @@ Reduction RedundancyElimination::Reduce(Node* node) {
       SIMPLIFIED_CHECKED_OP_LIST(SIMPLIFIED_CHECKED_OP)
 #undef SIMPLIFIED_CHECKED_OP
       return ReduceCheckNode(node);
+    case IrOpcode::kSpeculativeNumberEqual:
+    case IrOpcode::kSpeculativeNumberLessThan:
+    case IrOpcode::kSpeculativeNumberLessThanOrEqual:
+      return ReduceSpeculativeNumberComparison(node);
     case IrOpcode::kSpeculativeNumberAdd:
     case IrOpcode::kSpeculativeNumberSubtract:
     case IrOpcode::kSpeculativeSafeIntegerAdd:
@@ -269,6 +273,64 @@ Reduction RedundancyElimination::ReduceEffectPhi(Node* node) {
   return UpdateChecks(node, checks);
 }
 
+Reduction RedundancyElimination::ReduceSpeculativeNumberComparison(Node* node) {
+  NumberOperationHint const hint = NumberOperationHintOf(node->op());
+  Node* const first = NodeProperties::GetValueInput(node, 0);
+  Type const first_type = NodeProperties::GetType(first);
+  Node* const second = NodeProperties::GetValueInput(node, 1);
+  Type const second_type = NodeProperties::GetType(second);
+  Node* const effect = NodeProperties::GetEffectInput(node);
+  EffectPathChecks const* checks = node_checks_.Get(effect);
+
+  // If we do not know anything about the predecessor, do not propagate just yet
+  // because we will have to recompute anyway once we compute the predecessor.
+  if (checks == nullptr) return NoChange();
+
+  // Avoid the potentially expensive lookups below if the {node}
+  // has seen non-Smi inputs in the past, which is a clear signal
+  // that the comparison is probably not performed on a value that
+  // already passed an array bounds check.
+  if (hint == NumberOperationHint::kSignedSmall) {
+    // Don't bother trying to find a CheckBounds for the {first} input
+    // if it's type is already in UnsignedSmall range, since the bounds
+    // check is only going to narrow that range further, but the result
+    // is not going to make the representation selection any better.
+    if (!first_type.Is(Type::UnsignedSmall())) {
+      if (Node* check = checks->LookupBoundsCheckFor(first)) {
+        if (!first_type.Is(NodeProperties::GetType(check))) {
+          // Replace the {first} input with the {check}. This is safe,
+          // despite the fact that {check} can truncate -0 to 0, because
+          // the regular Number comparisons in JavaScript also identify
+          // 0 and -0 (unlike special comparisons as Object.is).
+          NodeProperties::ReplaceValueInput(node, check, 0);
+          Reduction const reduction = ReduceSpeculativeNumberComparison(node);
+          return reduction.Changed() ? reduction : Changed(node);
+        }
+      }
+    }
+
+    // Don't bother trying to find a CheckBounds for the {second} input
+    // if it's type is already in UnsignedSmall range, since the bounds
+    // check is only going to narrow that range further, but the result
+    // is not going to make the representation selection any better.
+    if (!second_type.Is(Type::UnsignedSmall())) {
+      if (Node* check = checks->LookupBoundsCheckFor(second)) {
+        if (!second_type.Is(NodeProperties::GetType(check))) {
+          // Replace the {second} input with the {check}. This is safe,
+          // despite the fact that {check} can truncate -0 to 0, because
+          // the regular Number comparisons in JavaScript also identify
+          // 0 and -0 (unlike special comparisons as Object.is).
+          NodeProperties::ReplaceValueInput(node, check, 1);
+          Reduction const reduction = ReduceSpeculativeNumberComparison(node);
+          return reduction.Changed() ? reduction : Changed(node);
+        }
+      }
+    }
+  }
+
+  return UpdateChecks(node, checks);
+}
+
 Reduction RedundancyElimination::ReduceSpeculativeNumberOperation(Node* node) {
   DCHECK(node->opcode() == IrOpcode::kSpeculativeNumberAdd ||
          node->opcode() == IrOpcode::kSpeculativeNumberSubtract ||

src/compiler/redundancy-elimination.h

@@ -59,6 +59,7 @@ class V8_EXPORT_PRIVATE RedundancyElimination final : public AdvancedReducer {
 
   Reduction ReduceCheckNode(Node* node);
   Reduction ReduceEffectPhi(Node* node);
+  Reduction ReduceSpeculativeNumberComparison(Node* node);
   Reduction ReduceSpeculativeNumberOperation(Node* node);
   Reduction ReduceStart(Node* node);
   Reduction ReduceOtherNode(Node* node);

test/mjsunit/compiler/redundancy-elimination.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --allow-natives-syntax
+// Flags: --allow-natives-syntax --opt
 
 // Test the RedundancyElimination::ReduceSpeculativeNumberOperation()
 // TurboFan optimization for the case of SpeculativeNumberAdd with
@@ -132,3 +132,63 @@
   %OptimizeFunctionOnNextCall(foo);
   assertEquals(3, foo([1, 2], 1));
 })();
+
+// Test the RedundancyElimination::ReduceSpeculativeNumberComparison()
+// TurboFan optimization for the case of SpeculativeNumberEqual.
+(function() {
+  function foo(a, i) {
+    const x = a[i];
+    if (i === 0) return x;
+    return i;
+  }
+
+  assertEquals(1, foo([1, 2], 0));
+  assertEquals(1, foo([1, 2], 1));
+  %OptimizeFunctionOnNextCall(foo);
+  assertEquals(1, foo([1, 2], 0));
+  assertEquals(1, foo([1, 2], 1));
+  // Even passing -0 should not deoptimize and
+  // of course still pass the equality test above.
+  assertEquals(9, foo([9, 2], -0));
+  assertOptimized(foo);
+})();
+
+// Test the RedundancyElimination::ReduceSpeculativeNumberComparison()
+// TurboFan optimization for the case of SpeculativeNumberLessThan.
+(function() {
+  function foo(a, i) {
+    const x = a[i];
+    if (i < 1) return x;
+    return i;
+  }
+
+  assertEquals(1, foo([1, 2], 0));
+  assertEquals(1, foo([1, 2], 1));
+  %OptimizeFunctionOnNextCall(foo);
+  assertEquals(1, foo([1, 2], 0));
+  assertEquals(1, foo([1, 2], 1));
+  // Even passing -0 should not deoptimize and
+  // of course still pass the equality test above.
+  assertEquals(9, foo([9, 2], -0));
+  assertOptimized(foo);
+})();
+
+// Test the RedundancyElimination::ReduceSpeculativeNumberComparison()
+// TurboFan optimization for the case of SpeculativeNumberLessThanOrEqual.
+(function() {
+  function foo(a, i) {
+    const x = a[i];
+    if (i <= 0) return x;
+    return i;
+  }
+
+  assertEquals(1, foo([1, 2], 0));
+  assertEquals(1, foo([1, 2], 1));
+  %OptimizeFunctionOnNextCall(foo);
+  assertEquals(1, foo([1, 2], 0));
+  assertEquals(1, foo([1, 2], 1));
+  // Even passing -0 should not deoptimize and
+  // of course still pass the equality test above.
+  assertEquals(9, foo([9, 2], -0));
+  assertOptimized(foo);
+})();

test/unittests/compiler/node-test-utils.cc

@@ -1948,6 +1948,9 @@ Matcher<Node*> IsTailCall(
         effect_matcher, control_matcher));                                    \
   }
 SIMPLIFIED_SPECULATIVE_NUMBER_BINOP_LIST(DEFINE_SPECULATIVE_BINOP_MATCHER);
+DEFINE_SPECULATIVE_BINOP_MATCHER(SpeculativeNumberEqual)
+DEFINE_SPECULATIVE_BINOP_MATCHER(SpeculativeNumberLessThan)
+DEFINE_SPECULATIVE_BINOP_MATCHER(SpeculativeNumberLessThanOrEqual)
 #undef DEFINE_SPECULATIVE_BINOP_MATCHER
 
 Matcher<Node*> IsStringConcat(const Matcher<Node*>& length_matcher,

test/unittests/compiler/node-test-utils.h

@@ -213,6 +213,9 @@ Matcher<Node*> IsNumberAdd(const Matcher<Node*>& lhs_matcher,
                             const Matcher<Node*>& effect_matcher,             \
                             const Matcher<Node*>& control_matcher);
 SIMPLIFIED_SPECULATIVE_NUMBER_BINOP_LIST(DECLARE_SPECULATIVE_BINOP_MATCHER);
+DECLARE_SPECULATIVE_BINOP_MATCHER(SpeculativeNumberEqual)
+DECLARE_SPECULATIVE_BINOP_MATCHER(SpeculativeNumberLessThan)
+DECLARE_SPECULATIVE_BINOP_MATCHER(SpeculativeNumberLessThanOrEqual)
 #undef DECLARE_SPECULATIVE_BINOP_MATCHER
 
 Matcher<Node*> IsNumberSubtract(const Matcher<Node*>& lhs_matcher,

test/unittests/compiler/redundancy-elimination-unittest.cc

@@ -646,6 +646,231 @@ TEST_F(RedundancyEliminationTest, CheckedUint64ToTaggedSigned) {
   }
 }
 
+// -----------------------------------------------------------------------------
+// SpeculativeNumberEqual
+
+TEST_F(RedundancyEliminationTest,
+       SpeculativeNumberEqualWithCheckBoundsBetterType) {
+  Typer typer(js_heap_broker(), Typer::kNoFlags, graph());
+  TRACED_FOREACH(VectorSlotPair, feedback1, vector_slot_pairs()) {
+    TRACED_FOREACH(VectorSlotPair, feedback2, vector_slot_pairs()) {
+      Node* lhs = Parameter(Type::Any(), 0);
+      Node* rhs = Parameter(Type::Any(), 1);
+      Node* length = Parameter(Type::Unsigned31(), 2);
+      Node* effect = graph()->start();
+      Node* control = graph()->start();
+
+      Node* check1 = effect = graph()->NewNode(
+          simplified()->CheckBounds(feedback1), lhs, length, effect, control);
+      Reduction r1 = Reduce(check1);
+      ASSERT_TRUE(r1.Changed());
+      EXPECT_EQ(r1.replacement(), check1);
+
+      Node* check2 = effect = graph()->NewNode(
+          simplified()->CheckBounds(feedback2), rhs, length, effect, control);
+      Reduction r2 = Reduce(check2);
+      ASSERT_TRUE(r2.Changed());
+      EXPECT_EQ(r2.replacement(), check2);
+
+      Node* cmp3 = effect =
+          graph()->NewNode(simplified()->SpeculativeNumberEqual(
+                               NumberOperationHint::kSignedSmall),
+                           lhs, rhs, effect, control);
+      Reduction r3 = Reduce(cmp3);
+      ASSERT_TRUE(r3.Changed());
+      EXPECT_THAT(r3.replacement(),
+                  IsSpeculativeNumberEqual(NumberOperationHint::kSignedSmall,
+                                           check1, check2, _, _));
+    }
+  }
+}
+
+TEST_F(RedundancyEliminationTest,
+       SpeculativeNumberEqualWithCheckBoundsSameType) {
+  Typer typer(js_heap_broker(), Typer::kNoFlags, graph());
+  TRACED_FOREACH(VectorSlotPair, feedback1, vector_slot_pairs()) {
+    TRACED_FOREACH(VectorSlotPair, feedback2, vector_slot_pairs()) {
+      Node* lhs = Parameter(Type::UnsignedSmall(), 0);
+      Node* rhs = Parameter(Type::UnsignedSmall(), 1);
+      Node* length = Parameter(Type::Unsigned31(), 2);
+      Node* effect = graph()->start();
+      Node* control = graph()->start();
+
+      Node* check1 = effect = graph()->NewNode(
+          simplified()->CheckBounds(feedback1), lhs, length, effect, control);
+      Reduction r1 = Reduce(check1);
+      ASSERT_TRUE(r1.Changed());
+      EXPECT_EQ(r1.replacement(), check1);
+
+      Node* check2 = effect = graph()->NewNode(
+          simplified()->CheckBounds(feedback2), rhs, length, effect, control);
+      Reduction r2 = Reduce(check2);
+      ASSERT_TRUE(r2.Changed());
+      EXPECT_EQ(r2.replacement(), check2);
+
+      Node* cmp3 = effect =
+          graph()->NewNode(simplified()->SpeculativeNumberEqual(
+                               NumberOperationHint::kSignedSmall),
+                           lhs, rhs, effect, control);
+      Reduction r3 = Reduce(cmp3);
+      ASSERT_TRUE(r3.Changed());
+      EXPECT_THAT(r3.replacement(),
+                  IsSpeculativeNumberEqual(NumberOperationHint::kSignedSmall,
+                                           lhs, rhs, _, _));
+    }
+  }
+}
+
+// -----------------------------------------------------------------------------
+// SpeculativeNumberLessThan
+
+TEST_F(RedundancyEliminationTest,
+       SpeculativeNumberLessThanWithCheckBoundsBetterType) {
+  Typer typer(js_heap_broker(), Typer::kNoFlags, graph());
+  TRACED_FOREACH(VectorSlotPair, feedback1, vector_slot_pairs()) {
+    TRACED_FOREACH(VectorSlotPair, feedback2, vector_slot_pairs()) {
+      Node* lhs = Parameter(Type::Any(), 0);
+      Node* rhs = Parameter(Type::Any(), 1);
+      Node* length = Parameter(Type::Unsigned31(), 2);
+      Node* effect = graph()->start();
+      Node* control = graph()->start();
+
+      Node* check1 = effect = graph()->NewNode(
+          simplified()->CheckBounds(feedback1), lhs, length, effect, control);
+      Reduction r1 = Reduce(check1);
+      ASSERT_TRUE(r1.Changed());
+      EXPECT_EQ(r1.replacement(), check1);
+
+      Node* check2 = effect = graph()->NewNode(
+          simplified()->CheckBounds(feedback2), rhs, length, effect, control);
+      Reduction r2 = Reduce(check2);
+      ASSERT_TRUE(r2.Changed());
+      EXPECT_EQ(r2.replacement(), check2);
+
+      Node* cmp3 = effect =
+          graph()->NewNode(simplified()->SpeculativeNumberLessThan(
+                               NumberOperationHint::kSignedSmall),
+                           lhs, rhs, effect, control);
+      Reduction r3 = Reduce(cmp3);
+      ASSERT_TRUE(r3.Changed());
+      EXPECT_THAT(r3.replacement(),
+                  IsSpeculativeNumberLessThan(NumberOperationHint::kSignedSmall,
+                                              check1, check2, _, _));
+    }
+  }
+}
+
+TEST_F(RedundancyEliminationTest,
+       SpeculativeNumberLessThanWithCheckBoundsSameType) {
+  Typer typer(js_heap_broker(), Typer::kNoFlags, graph());
+  TRACED_FOREACH(VectorSlotPair, feedback1, vector_slot_pairs()) {
+    TRACED_FOREACH(VectorSlotPair, feedback2, vector_slot_pairs()) {
+      Node* lhs = Parameter(Type::UnsignedSmall(), 0);
+      Node* rhs = Parameter(Type::UnsignedSmall(), 1);
+      Node* length = Parameter(Type::Unsigned31(), 2);
+      Node* effect = graph()->start();
+      Node* control = graph()->start();
+
+      Node* check1 = effect = graph()->NewNode(
+          simplified()->CheckBounds(feedback1), lhs, length, effect, control);
+      Reduction r1 = Reduce(check1);
+      ASSERT_TRUE(r1.Changed());
+      EXPECT_EQ(r1.replacement(), check1);
+
+      Node* check2 = effect = graph()->NewNode(
+          simplified()->CheckBounds(feedback2), rhs, length, effect, control);
+      Reduction r2 = Reduce(check2);
+      ASSERT_TRUE(r2.Changed());
+      EXPECT_EQ(r2.replacement(), check2);
+
+      Node* cmp3 = effect =
+          graph()->NewNode(simplified()->SpeculativeNumberLessThan(
+                               NumberOperationHint::kSignedSmall),
+                           lhs, rhs, effect, control);
+      Reduction r3 = Reduce(cmp3);
+      ASSERT_TRUE(r3.Changed());
+      EXPECT_THAT(r3.replacement(),
+                  IsSpeculativeNumberLessThan(NumberOperationHint::kSignedSmall,
+                                              lhs, rhs, _, _));
+    }
+  }
+}
+
+// -----------------------------------------------------------------------------
+// SpeculativeNumberLessThanOrEqual
+
+TEST_F(RedundancyEliminationTest,
+       SpeculativeNumberLessThanOrEqualWithCheckBoundsBetterType) {
+  Typer typer(js_heap_broker(), Typer::kNoFlags, graph());
+  TRACED_FOREACH(VectorSlotPair, feedback1, vector_slot_pairs()) {
+    TRACED_FOREACH(VectorSlotPair, feedback2, vector_slot_pairs()) {
+      Node* lhs = Parameter(Type::Any(), 0);
+      Node* rhs = Parameter(Type::Any(), 1);
+      Node* length = Parameter(Type::Unsigned31(), 2);
+      Node* effect = graph()->start();
+      Node* control = graph()->start();
+
+      Node* check1 = effect = graph()->NewNode(
+          simplified()->CheckBounds(feedback1), lhs, length, effect, control);
+      Reduction r1 = Reduce(check1);
+      ASSERT_TRUE(r1.Changed());
+      EXPECT_EQ(r1.replacement(), check1);
+
+      Node* check2 = effect = graph()->NewNode(
+          simplified()->CheckBounds(feedback2), rhs, length, effect, control);
+      Reduction r2 = Reduce(check2);
+      ASSERT_TRUE(r2.Changed());
+      EXPECT_EQ(r2.replacement(), check2);
+
+      Node* cmp3 = effect =
+          graph()->NewNode(simplified()->SpeculativeNumberLessThanOrEqual(
+                               NumberOperationHint::kSignedSmall),
+                           lhs, rhs, effect, control);
+      Reduction r3 = Reduce(cmp3);
+      ASSERT_TRUE(r3.Changed());
+      EXPECT_THAT(r3.replacement(),
+                  IsSpeculativeNumberLessThanOrEqual(
+                      NumberOperationHint::kSignedSmall, check1, check2, _, _));
+    }
+  }
+}
+
+TEST_F(RedundancyEliminationTest,
+       SpeculativeNumberLessThanOrEqualWithCheckBoundsSameType) {
+  Typer typer(js_heap_broker(), Typer::kNoFlags, graph());
+  TRACED_FOREACH(VectorSlotPair, feedback1, vector_slot_pairs()) {
+    TRACED_FOREACH(VectorSlotPair, feedback2, vector_slot_pairs()) {
+      Node* lhs = Parameter(Type::UnsignedSmall(), 0);
+      Node* rhs = Parameter(Type::UnsignedSmall(), 1);
+      Node* length = Parameter(Type::Unsigned31(), 2);
+      Node* effect = graph()->start();
+      Node* control = graph()->start();
+
+      Node* check1 = effect = graph()->NewNode(
+          simplified()->CheckBounds(feedback1), lhs, length, effect, control);
+      Reduction r1 = Reduce(check1);
+      ASSERT_TRUE(r1.Changed());
+      EXPECT_EQ(r1.replacement(), check1);
+
+      Node* check2 = effect = graph()->NewNode(
+          simplified()->CheckBounds(feedback2), rhs, length, effect, control);
+      Reduction r2 = Reduce(check2);
+      ASSERT_TRUE(r2.Changed());
+      EXPECT_EQ(r2.replacement(), check2);
+
+      Node* cmp3 = effect =
+          graph()->NewNode(simplified()->SpeculativeNumberLessThanOrEqual(
+                               NumberOperationHint::kSignedSmall),
+                           lhs, rhs, effect, control);
+      Reduction r3 = Reduce(cmp3);
+      ASSERT_TRUE(r3.Changed());
+      EXPECT_THAT(r3.replacement(),
+                  IsSpeculativeNumberLessThanOrEqual(
+                      NumberOperationHint::kSignedSmall, lhs, rhs, _, _));
+    }
+  }
+}
+
 // -----------------------------------------------------------------------------
 // SpeculativeNumberAdd