[turbofan] Improve equality on NumberOrOddball

This CL cleans up CompareOperationFeedback by replacing it with a
composable set of flags. The interpreter is changed to collect
more specific feedback for abstract equality, especially if oddballs
are involved.

TurboFan is changed to construct SpeculativeNumberEqual operator
instead of the generic JSEqual in many more cases. This change has
shown a local speedup of a factor of 3-10, because the specific
operator is way faster than calling into the generic builtin, but
it also enables additional optimizations, further improving
runtime performance.

Bug: v8:5660
Change-Id: I856752caa707e9a4f742c6e7a9c75552fb431d28
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2162854Reviewed-by: Mythri Alle <mythria@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Nico Hartmann <nicohartmann@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67645}

src/common/globals.h

@@ -1348,31 +1348,46 @@ class BinaryOperationFeedback {
 };
 
 // Type feedback is encoded in such a way that, we can combine the feedback
-// at different points by performing an 'OR' operation. Type feedback moves
-// to a more generic type when we combine feedback.
-//
-//   kSignedSmall -> kNumber             -> kNumberOrOddball           -> kAny
-//                   kReceiver           -> kReceiverOrNullOrUndefined -> kAny
-//                   kInternalizedString -> kString                    -> kAny
-//                                          kSymbol                    -> kAny
-//                                          kBigInt                    -> kAny
-//
+// at different points by performing an 'OR' operation.
 // This is distinct from BinaryOperationFeedback on purpose, because the
 // feedback that matters differs greatly as well as the way it is consumed.
 class CompareOperationFeedback {
- public:
   enum {
-    kNone = 0x000,
-    kSignedSmall = 0x001,
-    kNumber = 0x003,
-    kNumberOrOddball = 0x007,
-    kInternalizedString = 0x008,
-    kString = 0x018,
-    kSymbol = 0x020,
-    kBigInt = 0x040,
-    kReceiver = 0x080,
-    kReceiverOrNullOrUndefined = 0x180,
-    kAny = 0x1ff
+    kSignedSmallFlag = 1 << 0,
+    kOtherNumberFlag = 1 << 1,
+    kBooleanFlag = 1 << 2,
+    kOtherOddballFlag = 1 << 3,
+    kNullOrUndefinedFlag = 1 << 4,
+    kInternalizedStringFlag = 1 << 5,
+    kOtherStringFlag = 1 << 6,
+    kSymbolFlag = 1 << 7,
+    kBigIntFlag = 1 << 8,
+    kReceiverFlag = 1 << 9,
+    kAnyMask = 0x3FF,
+  };
+
+ public:
+  enum Type {
+    kNone = 0,
+
+    kBoolean = kBooleanFlag,
+    kNullOrUndefined = kNullOrUndefinedFlag,
+    kOddball = kBoolean | kOtherOddballFlag | kNullOrUndefined,
+
+    kSignedSmall = kSignedSmallFlag,
+    kNumber = kSignedSmall | kOtherNumberFlag,
+    kNumberOrOddball = kNumber | kOddball,
+
+    kInternalizedString = kInternalizedStringFlag,
+    kString = kInternalizedString | kOtherStringFlag,
+
+    kReceiver = kReceiverFlag,
+    kReceiverOrNullOrUndefined = kReceiver | kNullOrUndefined,
+
+    kBigInt = kBigIntFlag,
+    kSymbol = kSymbolFlag,
+
+    kAny = kAnyMask,
   };
 };
 

src/compiler/code-assembler.cc

@@ -1393,6 +1393,7 @@ void CodeAssemblerLabel::MergeVariables() {
     }
     // If the following asserts, then you've jumped to a label without a bound
     // variable along that path that expects to merge its value into a phi.
+    // This can also occur if a label is bound that is never jumped to.
     DCHECK(variable_phis_.find(var) == variable_phis_.end() ||
            count == merge_count_);
     USE(count);

src/compiler/js-typed-lowering.cc

@@ -878,7 +878,12 @@ Reduction JSTypedLowering::ReduceJSStrictEqual(Node* node) {
   if (r.BothInputsAre(Type::Signed32()) ||
       r.BothInputsAre(Type::Unsigned32())) {
     return r.ChangeToPureOperator(simplified()->NumberEqual());
-  } else if (r.GetCompareNumberOperationHint(&hint)) {
+  } else if (r.GetCompareNumberOperationHint(&hint) &&
+             hint != NumberOperationHint::kNumberOrOddball) {
+    // SpeculativeNumberEqual[kNumberOrOddball] performs implicit conversion
+    // of oddballs to numbers, so we must not generate it for strict equality.
+    DCHECK(hint == NumberOperationHint::kNumber ||
+           hint == NumberOperationHint::kSignedSmall);
     return r.ChangeToSpeculativeOperator(
         simplified()->SpeculativeNumberEqual(hint), Type::Boolean());
   } else if (r.BothInputsAre(Type::Number())) {

src/compiler/simplified-lowering.cc

@@ -2054,11 +2054,6 @@ class RepresentationSelector {
             // This doesn't make sense for compare operations.
             UNREACHABLE();
           case NumberOperationHint::kNumberOrOddball:
-            // Abstract and strict equality don't perform ToNumber conversions
-            // on Oddballs, so make sure we don't accidentially sneak in a
-            // hint with Oddball feedback here.
-            DCHECK_NE(IrOpcode::kSpeculativeNumberEqual, node->opcode());
-            V8_FALLTHROUGH;
           case NumberOperationHint::kNumber:
             VisitBinop<T>(node,
                           CheckedUseInfoAsFloat64FromHint(

src/objects/feedback-vector-inl.h

@@ -233,32 +233,47 @@ BinaryOperationHint BinaryOperationHintFromFeedback(int type_feedback) {
 }
 
 // Helper function to transform the feedback to CompareOperationHint.
+template <CompareOperationFeedback::Type Feedback>
+bool Is(int type_feedback) {
+  return !(type_feedback & ~Feedback);
+}
+
 CompareOperationHint CompareOperationHintFromFeedback(int type_feedback) {
-  switch (type_feedback) {
-    case CompareOperationFeedback::kNone:
-      return CompareOperationHint::kNone;
-    case CompareOperationFeedback::kSignedSmall:
-      return CompareOperationHint::kSignedSmall;
-    case CompareOperationFeedback::kNumber:
-      return CompareOperationHint::kNumber;
-    case CompareOperationFeedback::kNumberOrOddball:
-      return CompareOperationHint::kNumberOrOddball;
-    case CompareOperationFeedback::kInternalizedString:
-      return CompareOperationHint::kInternalizedString;
-    case CompareOperationFeedback::kString:
-      return CompareOperationHint::kString;
-    case CompareOperationFeedback::kSymbol:
-      return CompareOperationHint::kSymbol;
-    case CompareOperationFeedback::kBigInt:
-      return CompareOperationHint::kBigInt;
-    case CompareOperationFeedback::kReceiver:
-      return CompareOperationHint::kReceiver;
-    case CompareOperationFeedback::kReceiverOrNullOrUndefined:
-      return CompareOperationHint::kReceiverOrNullOrUndefined;
-    default:
-      return CompareOperationHint::kAny;
+  if (Is<CompareOperationFeedback::kNone>(type_feedback)) {
+    return CompareOperationHint::kNone;
   }
-  UNREACHABLE();
+
+  if (Is<CompareOperationFeedback::kSignedSmall>(type_feedback)) {
+    return CompareOperationHint::kSignedSmall;
+  } else if (Is<CompareOperationFeedback::kNumber>(type_feedback)) {
+    return CompareOperationHint::kNumber;
+  } else if (Is<CompareOperationFeedback::kNumberOrOddball>(type_feedback)) {
+    return CompareOperationHint::kNumberOrOddball;
+  }
+
+  if (Is<CompareOperationFeedback::kInternalizedString>(type_feedback)) {
+    return CompareOperationHint::kInternalizedString;
+  } else if (Is<CompareOperationFeedback::kString>(type_feedback)) {
+    return CompareOperationHint::kString;
+  }
+
+  if (Is<CompareOperationFeedback::kReceiver>(type_feedback)) {
+    return CompareOperationHint::kReceiver;
+  } else if (Is<CompareOperationFeedback::kReceiverOrNullOrUndefined>(
+                 type_feedback)) {
+    return CompareOperationHint::kReceiverOrNullOrUndefined;
+  }
+
+  if (Is<CompareOperationFeedback::kBigInt>(type_feedback)) {
+    return CompareOperationHint::kBigInt;
+  }
+
+  if (Is<CompareOperationFeedback::kSymbol>(type_feedback)) {
+    return CompareOperationHint::kSymbol;
+  }
+
+  DCHECK(Is<CompareOperationFeedback::kAny>(type_feedback));
+  return CompareOperationHint::kAny;
 }
 
 // Helper function to transform the feedback to ForInHint.

test/cctest/interpreter/test-interpreter.cc

@@ -2087,7 +2087,6 @@ TEST(InterpreterMixedComparisons) {
                 LoadStringAndAddSpace(&builder, &ast_factory, rhs_cstr,
                                       string_add_slot);
               }
-              break;
             } else {
               CHECK_EQ(which_side, kLhsIsString);
               // Comparison with String on the lhs and HeapNumber on the rhs.
@@ -2121,9 +2120,19 @@ TEST(InterpreterMixedComparisons) {
             if (tester.HasFeedbackMetadata()) {
               MaybeObject feedback = callable.vector().Get(slot);
               CHECK(feedback->IsSmi());
-              // Comparison with a number and string collects kAny feedback.
-              CHECK_EQ(CompareOperationFeedback::kAny,
-                       feedback->ToSmi().value());
+              if (kComparisonTypes[c] == Token::Value::EQ) {
+                // For sloppy equality, we have more precise feedback.
+                CHECK_EQ(
+                    CompareOperationFeedback::kNumber |
+                        (string_type == kInternalizedStringConstant
+                             ? CompareOperationFeedback::kInternalizedString
+                             : CompareOperationFeedback::kString),
+                    feedback->ToSmi().value());
+              } else {
+                // Comparison with a number and string collects kAny feedback.
+                CHECK_EQ(CompareOperationFeedback::kAny,
+                         feedback->ToSmi().value());
+              }
             }
           }
         }

test/mjsunit/regress/regress-5660.js

+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --opt --allow-natives-syntax
+
+(function() {
+  function test(a, b) {
+    return a === b;
+  }
+
+  %PrepareFunctionForOptimization(test);
+  assertTrue(test(undefined, undefined));
+  assertTrue(test(undefined, undefined));
+  %OptimizeFunctionOnNextCall(test);
+  assertTrue(test(undefined, undefined));
+})();
+
+(function() {
+  function test(a, b) {
+    return a === b;
+  }
+
+  %PrepareFunctionForOptimization(test);
+  assertTrue(test(true, true));
+  assertTrue(test(true, true));
+  %OptimizeFunctionOnNextCall(test);
+  assertFalse(test(true, 1));
+})();
+
+(function() {
+  function test(a, b) {
+      return a == b;
+  }
+
+  %PrepareFunctionForOptimization(test);
+  assertTrue(test(true, true));
+  assertTrue(test(true, true));
+  %OptimizeFunctionOnNextCall(test);
+  assertTrue(test(true, 1));
+})();