[Interpreter] Collect String feedback on CompareOps.

Collect string feedback for compare operations. Without this,
functions which have a lot of string compare operations end up with
a high generic type percentage, and don't get optimized until very
late.

Currently TurboFan doesn't use this String feedback for compare
operations, but this could be done in future work if it is useful.

BUG=chromium:660947

Review-Url: https://codereview.chromium.org/2506013005
Cr-Commit-Position: refs/heads/master@{#41078}

src/compiler/js-operator.cc

@@ -526,6 +526,7 @@ struct JSOperatorGlobalCache final {
   Name##Operator<CompareOperationHint::kNumber> k##Name##NumberOperator;  \
   Name##Operator<CompareOperationHint::kNumberOrOddball>                  \
       k##Name##NumberOrOddballOperator;                                   \
+  Name##Operator<CompareOperationHint::kString> k##Name##StringOperator;  \
   Name##Operator<CompareOperationHint::kAny> k##Name##AnyOperator;
   COMPARE_OP_LIST(COMPARE_OP)
 #undef COMPARE_OP
@@ -577,6 +578,8 @@ BINARY_OP_LIST(BINARY_OP)
         return &cache_.k##Name##NumberOperator;                        \
       case CompareOperationHint::kNumberOrOddball:                     \
         return &cache_.k##Name##NumberOrOddballOperator;               \
+      case CompareOperationHint::kString:                              \
+        return &cache_.k##Name##StringOperator;                        \
       case CompareOperationHint::kAny:                                 \
         return &cache_.k##Name##AnyOperator;                           \
     }                                                                  \

src/compiler/js-typed-lowering.cc

@@ -69,6 +69,7 @@ class JSBinopReduction final {
           return true;
         case CompareOperationHint::kAny:
         case CompareOperationHint::kNone:
+        case CompareOperationHint::kString:
           break;
       }
     }

src/compiler/type-hint-analyzer.cc

@@ -50,6 +50,7 @@ CompareOperationHint ToCompareOperationHint(Token::Value op,
                  : CompareOperationHint::kNumber;
     case CompareICState::STRING:
     case CompareICState::INTERNALIZED_STRING:
+      return CompareOperationHint::kString;
     case CompareICState::UNIQUE_NAME:
     case CompareICState::RECEIVER:
     case CompareICState::KNOWN_RECEIVER:

src/globals.h

@@ -1241,10 +1241,21 @@ 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  -> kAny
+//                 kString  -> kAny
 // TODO(epertoso): consider unifying this with BinaryOperationFeedback.
 class CompareOperationFeedback {
  public:
-  enum { kNone = 0x00, kSignedSmall = 0x01, kNumber = 0x3, kAny = 0x7 };
+  enum {
+    kNone = 0x00,
+    kSignedSmall = 0x01,
+    kNumber = 0x3,
+    kString = 0x4,
+    kAny = 0xf
+  };
 };
 
 // Describes how exactly a frame has been dropped from stack.

src/interpreter/interpreter.cc

@@ -1000,62 +1000,91 @@ void Interpreter::DoCompareOpWithFeedback(Token::Value compare_op,
   // sometimes emit comparisons that shouldn't collect feedback (e.g.
   // try-finally blocks and generators), and we could get rid of this by
   // introducing Smi equality tests.
-  Label skip_feedback_update(assembler);
-  __ GotoIf(__ WordEqual(slot_index, __ IntPtrConstant(0)),
-            &skip_feedback_update);
+  Label gather_type_feedback(assembler), do_compare(assembler);
+  __ Branch(__ WordEqual(slot_index, __ IntPtrConstant(0)), &do_compare,
+            &gather_type_feedback);
 
-  Variable var_type_feedback(assembler, MachineRepresentation::kWord32);
-  Label lhs_is_smi(assembler), lhs_is_not_smi(assembler),
-      gather_rhs_type(assembler), do_compare(assembler);
-  __ Branch(__ TaggedIsSmi(lhs), &lhs_is_smi, &lhs_is_not_smi);
-
-  __ Bind(&lhs_is_smi);
-  var_type_feedback.Bind(
-      __ Int32Constant(CompareOperationFeedback::kSignedSmall));
-  __ Goto(&gather_rhs_type);
-
-  __ Bind(&lhs_is_not_smi);
+  __ Bind(&gather_type_feedback);
   {
-    Label lhs_is_number(assembler), lhs_is_not_number(assembler);
-    Node* lhs_map = __ LoadMap(lhs);
-    __ Branch(__ WordEqual(lhs_map, __ HeapNumberMapConstant()), &lhs_is_number,
-              &lhs_is_not_number);
+    Variable var_type_feedback(assembler, MachineRepresentation::kWord32);
+    Label lhs_is_not_smi(assembler), lhs_is_not_number(assembler),
+        lhs_is_not_string(assembler), gather_rhs_type(assembler),
+        update_feedback(assembler);
+
+    __ GotoUnless(__ TaggedIsSmi(lhs), &lhs_is_not_smi);
 
-    __ Bind(&lhs_is_number);
-    var_type_feedback.Bind(__ Int32Constant(CompareOperationFeedback::kNumber));
+    var_type_feedback.Bind(
+        __ Int32Constant(CompareOperationFeedback::kSignedSmall));
     __ Goto(&gather_rhs_type);
 
-    __ Bind(&lhs_is_not_number);
-    var_type_feedback.Bind(__ Int32Constant(CompareOperationFeedback::kAny));
-    __ Goto(&do_compare);
-  }
+    __ Bind(&lhs_is_not_smi);
+    {
+      Node* lhs_map = __ LoadMap(lhs);
+      __ GotoUnless(__ WordEqual(lhs_map, __ HeapNumberMapConstant()),
+                    &lhs_is_not_number);
+
+      var_type_feedback.Bind(
+          __ Int32Constant(CompareOperationFeedback::kNumber));
+      __ Goto(&gather_rhs_type);
+
+      __ Bind(&lhs_is_not_number);
+      {
+        Node* lhs_instance_type = __ LoadInstanceType(lhs);
+        Node* lhs_type =
+            __ Select(__ IsStringInstanceType(lhs_instance_type),
+                      __ Int32Constant(CompareOperationFeedback::kString),
+                      __ Int32Constant(CompareOperationFeedback::kAny));
+
+        var_type_feedback.Bind(lhs_type);
+        __ Goto(&gather_rhs_type);
+      }
+    }
 
-  __ Bind(&gather_rhs_type);
-  {
-    Label rhs_is_smi(assembler);
-    __ GotoIf(__ TaggedIsSmi(rhs), &rhs_is_smi);
-
-    Node* rhs_map = __ LoadMap(rhs);
-    Node* rhs_type =
-        __ Select(__ WordEqual(rhs_map, __ HeapNumberMapConstant()),
-                  __ Int32Constant(CompareOperationFeedback::kNumber),
-                  __ Int32Constant(CompareOperationFeedback::kAny));
-    var_type_feedback.Bind(__ Word32Or(var_type_feedback.value(), rhs_type));
-    __ Goto(&do_compare);
-
-    __ Bind(&rhs_is_smi);
-    var_type_feedback.Bind(
-        __ Word32Or(var_type_feedback.value(),
-                    __ Int32Constant(CompareOperationFeedback::kSignedSmall)));
-    __ Goto(&do_compare);
+    __ Bind(&gather_rhs_type);
+    {
+      Label rhs_is_not_smi(assembler), rhs_is_not_number(assembler);
+
+      __ GotoUnless(__ TaggedIsSmi(rhs), &rhs_is_not_smi);
+
+      var_type_feedback.Bind(__ Word32Or(
+          var_type_feedback.value(),
+          __ Int32Constant(CompareOperationFeedback::kSignedSmall)));
+      __ Goto(&update_feedback);
+
+      __ Bind(&rhs_is_not_smi);
+      {
+        Node* rhs_map = __ LoadMap(rhs);
+        __ GotoUnless(__ WordEqual(rhs_map, __ HeapNumberMapConstant()),
+                      &rhs_is_not_number);
+
+        var_type_feedback.Bind(
+            __ Word32Or(var_type_feedback.value(),
+                        __ Int32Constant(CompareOperationFeedback::kNumber)));
+        __ Goto(&update_feedback);
+
+        __ Bind(&rhs_is_not_number);
+        {
+          Node* rhs_instance_type = __ LoadInstanceType(rhs);
+          Node* rhs_type =
+              __ Select(__ IsStringInstanceType(rhs_instance_type),
+                        __ Int32Constant(CompareOperationFeedback::kString),
+                        __ Int32Constant(CompareOperationFeedback::kAny));
+          var_type_feedback.Bind(
+              __ Word32Or(var_type_feedback.value(), rhs_type));
+          __ Goto(&update_feedback);
+        }
+      }
+    }
+
+    __ Bind(&update_feedback);
+    {
+      __ UpdateFeedback(var_type_feedback.value(), type_feedback_vector,
+                        slot_index);
+      __ Goto(&do_compare);
+    }
   }
 
   __ Bind(&do_compare);
-  __ UpdateFeedback(var_type_feedback.value(), type_feedback_vector,
-                    slot_index);
-  __ Goto(&skip_feedback_update);
-
-  __ Bind(&skip_feedback_update);
   Node* result;
   switch (compare_op) {
     case Token::EQ:

src/type-feedback-vector-inl.h

@@ -149,6 +149,8 @@ CompareOperationHint CompareOperationHintFromFeedback(int type_feedback) {
       return CompareOperationHint::kSignedSmall;
     case CompareOperationFeedback::kNumber:
       return CompareOperationHint::kNumber;
+    case CompareOperationFeedback::kString:
+      return CompareOperationHint::kString;
     default:
       return CompareOperationHint::kAny;
   }

src/type-hints.cc

@@ -36,6 +36,8 @@ std::ostream& operator<<(std::ostream& os, CompareOperationHint hint) {
       return os << "Number";
     case CompareOperationHint::kNumberOrOddball:
       return os << "NumberOrOddball";
+    case CompareOperationHint::kString:
+      return os << "String";
     case CompareOperationHint::kAny:
       return os << "Any";
   }

src/type-hints.h

@@ -33,6 +33,7 @@ enum class CompareOperationHint : uint8_t {
   kSignedSmall,
   kNumber,
   kNumberOrOddball,
+  kString,
   kAny
 };
 

src/type-info.cc

@@ -203,6 +203,8 @@ AstType* CompareOpHintToType(CompareOperationHint hint) {
       return AstType::Number();
     case CompareOperationHint::kNumberOrOddball:
       return AstType::NumberOrOddball();
+    case CompareOperationHint::kString:
+      return AstType::String();
     case CompareOperationHint::kAny:
       return AstType::Any();
   }

test/cctest/interpreter/test-interpreter.cc

@@ -555,6 +555,10 @@ TEST(InterpreterBinaryOpTypeFeedback) {
        isolate->factory()->NewHeapNumber(1.4142),
        isolate->factory()->NewHeapNumber(3.1415 + 1.4142),
        BinaryOperationFeedback::kNumber},
+      {Token::Value::ADD, isolate->factory()->NewStringFromAsciiChecked("foo"),
+       isolate->factory()->NewStringFromAsciiChecked("bar"),
+       isolate->factory()->NewStringFromAsciiChecked("foobar"),
+       BinaryOperationFeedback::kString},
       {Token::Value::ADD, Handle<Smi>(Smi::FromInt(2), isolate),
        isolate->factory()->NewStringFromAsciiChecked("2"),
        isolate->factory()->NewStringFromAsciiChecked("22"),
@@ -1809,7 +1813,7 @@ TEST(InterpreterStringComparisons) {
                  CompareC(comparison, inputs[i], inputs[j]));
         Object* feedback = vector->Get(slot);
         CHECK(feedback->IsSmi());
-        CHECK_EQ(CompareOperationFeedback::kAny,
+        CHECK_EQ(CompareOperationFeedback::kString,
                  static_cast<Smi*>(feedback)->value());
       }
     }
@@ -1822,7 +1826,7 @@ TEST(InterpreterMixedComparisons) {
   // convertible to a HeapNumber so comparison will be between numeric
   // values except for the strict comparisons where no conversion is
   // performed.
-  const char* inputs[] = {"-1.77", "-40.333", "0.01", "55.77e5", "2.01"};
+  const char* inputs[] = {"-1.77", "-40.333", "0.01", "55.77e50", "2.01"};
 
   UnicodeCache unicode_cache;
 
@@ -1875,8 +1879,10 @@ TEST(InterpreterMixedComparisons) {
                    CompareC(comparison, lhs, rhs, true));
           Object* feedback = vector->Get(slot);
           CHECK(feedback->IsSmi());
-          CHECK_EQ(CompareOperationFeedback::kAny,
-                   static_cast<Smi*>(feedback)->value());
+          // kNumber | kString gets converted to CompareOperationHint::kAny.
+          int expected_feedback = CompareOperationFeedback::kNumber |
+                                  CompareOperationFeedback::kString;
+          CHECK_EQ(expected_feedback, static_cast<Smi*>(feedback)->value());
         }
       }
     }