Revert of [interpreter] Add string type feedback to add (patchset #3 id:40001...

Revert of [interpreter] Add string type feedback to add (patchset #3 id:40001 of https://codereview.chromium.org/2392533002/ )

Reason for revert:
Fails unittests on win32 debug:
https://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20debug/builds/5026

Original issue's description:
> [interpreter] Add string type feedback to add
>
> Adds string type feedback to Ignition's AddWithFeedback code stub, for now only
> adding a special case for when both lhs and rhs are strings. This improves
> octane's splay by >100%.
>
> BUG=v8:5400
>
> Committed: https://crrev.com/fb4ae2239d37adaf0321165034050316914de708
> Cr-Commit-Position: refs/heads/master@{#39987}

TBR=rmcilroy@chromium.org,mythria@chromium.org,leszeks@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:5400

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

src/code-stubs.cc

@@ -1032,7 +1032,7 @@ compiler::Node* AddWithFeedbackStub::Generate(
 
   // Shared entry for floating point addition.
   Label do_fadd(assembler), end(assembler),
-      do_add_any(assembler, Label::kDeferred), call_add_stub(assembler);
+      call_add_stub(assembler, Label::kDeferred);
   Variable var_fadd_lhs(assembler, MachineRepresentation::kFloat64),
       var_fadd_rhs(assembler, MachineRepresentation::kFloat64),
       var_type_feedback(assembler, MachineRepresentation::kWord32),
@@ -1080,7 +1080,8 @@ compiler::Node* AddWithFeedbackStub::Generate(
       Node* rhs_map = assembler->LoadMap(rhs);
 
       // Check if the {rhs} is a HeapNumber.
-      assembler->GotoUnless(assembler->IsHeapNumberMap(rhs_map), &do_add_any);
+      assembler->GotoUnless(assembler->IsHeapNumberMap(rhs_map),
+                            &call_add_stub);
 
       var_fadd_lhs.Bind(assembler->SmiToFloat64(lhs));
       var_fadd_rhs.Bind(assembler->LoadHeapNumberValue(rhs));
@@ -1090,14 +1091,12 @@ compiler::Node* AddWithFeedbackStub::Generate(
 
   assembler->Bind(&if_lhsisnotsmi);
   {
-    Label check_string(assembler);
-
     // Load the map of {lhs}.
     Node* lhs_map = assembler->LoadMap(lhs);
 
     // Check if {lhs} is a HeapNumber.
     Label if_lhsisnumber(assembler), if_lhsisnotnumber(assembler);
-    assembler->GotoUnless(assembler->IsHeapNumberMap(lhs_map), &check_string);
+    assembler->GotoUnless(assembler->IsHeapNumberMap(lhs_map), &call_add_stub);
 
     // Check if the {rhs} is Smi.
     Label if_rhsissmi(assembler), if_rhsisnotsmi(assembler);
@@ -1116,34 +1115,13 @@ compiler::Node* AddWithFeedbackStub::Generate(
       Node* rhs_map = assembler->LoadMap(rhs);
 
       // Check if the {rhs} is a HeapNumber.
-      assembler->GotoUnless(assembler->IsHeapNumberMap(rhs_map), &do_add_any);
+      assembler->GotoUnless(assembler->IsHeapNumberMap(rhs_map),
+                            &call_add_stub);
 
       var_fadd_lhs.Bind(assembler->LoadHeapNumberValue(lhs));
       var_fadd_rhs.Bind(assembler->LoadHeapNumberValue(rhs));
       assembler->Goto(&do_fadd);
     }
-
-    assembler->Bind(&check_string);
-    {
-      // Check if the {rhs} is a smi, and exit the string check early if it is.
-      assembler->GotoIf(assembler->WordIsSmi(rhs), &do_add_any);
-
-      Node* lhs_instance_type = assembler->LoadMapInstanceType(lhs_map);
-
-      // Exit unless {lhs} is a string
-      assembler->GotoUnless(assembler->IsStringInstanceType(lhs_instance_type),
-                            &do_add_any);
-
-      Node* rhs_instance_type = assembler->LoadInstanceType(rhs);
-
-      // Exit unless {rhs} is a string
-      assembler->GotoUnless(assembler->IsStringInstanceType(rhs_instance_type),
-                            &do_add_any);
-
-      var_type_feedback.Bind(
-          assembler->Int32Constant(BinaryOperationFeedback::kString));
-      assembler->Goto(&call_add_stub);
-    }
   }
 
   assembler->Bind(&do_fadd);
@@ -1157,15 +1135,10 @@ compiler::Node* AddWithFeedbackStub::Generate(
     assembler->Goto(&end);
   }
 
-  assembler->Bind(&do_add_any);
+  assembler->Bind(&call_add_stub);
   {
     var_type_feedback.Bind(
         assembler->Int32Constant(BinaryOperationFeedback::kAny));
-    assembler->Goto(&call_add_stub);
-  }
-
-  assembler->Bind(&call_add_stub);
-  {
     Callable callable = CodeFactory::Add(assembler->isolate());
     var_result.Bind(assembler->CallStub(callable, context, lhs, rhs));
     assembler->Goto(&end);

src/globals.h

@@ -1209,16 +1209,9 @@ inline uint32_t ObjectHash(Address address) {
 // 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
 class BinaryOperationFeedback {
  public:
-  enum {
-    kNone = 0x0,
-    kSignedSmall = 0x1,
-    kNumber = 0x3,
-    kString = 0x4,
-    kAny = 0xF
-  };
+  enum { kNone = 0x00, kSignedSmall = 0x01, kNumber = 0x3, kAny = 0x7 };
 };
 
 // TODO(epertoso): consider unifying this with BinaryOperationFeedback.

src/type-feedback-vector-inl.h

@@ -129,8 +129,6 @@ BinaryOperationHint BinaryOperationHintFromFeedback(int type_feedback) {
       return BinaryOperationHint::kSignedSmall;
     case BinaryOperationFeedback::kNumber:
       return BinaryOperationHint::kNumberOrOddball;
-    case BinaryOperationFeedback::kString:
-      return BinaryOperationHint::kString;
     case BinaryOperationFeedback::kAny:
     default:
       return BinaryOperationHint::kAny;

test/cctest/interpreter/test-interpreter.cc

@@ -386,36 +386,28 @@ TEST(InterpreterStringAdd) {
     Handle<Object> lhs;
     Handle<Object> rhs;
     Handle<Object> expected_value;
-    int32_t expected_feedback;
   } test_cases[] = {
       {factory->NewStringFromStaticChars("a"),
        factory->NewStringFromStaticChars("b"),
-       factory->NewStringFromStaticChars("ab"),
-       BinaryOperationFeedback::kString},
+       factory->NewStringFromStaticChars("ab")},
       {factory->NewStringFromStaticChars("aaaaaa"),
        factory->NewStringFromStaticChars("b"),
-       factory->NewStringFromStaticChars("aaaaaab"),
-       BinaryOperationFeedback::kString},
+       factory->NewStringFromStaticChars("aaaaaab")},
       {factory->NewStringFromStaticChars("aaa"),
        factory->NewStringFromStaticChars("bbbbb"),
-       factory->NewStringFromStaticChars("aaabbbbb"),
-       BinaryOperationFeedback::kString},
+       factory->NewStringFromStaticChars("aaabbbbb")},
       {factory->NewStringFromStaticChars(""),
        factory->NewStringFromStaticChars("b"),
-       factory->NewStringFromStaticChars("b"),
-       BinaryOperationFeedback::kString},
+       factory->NewStringFromStaticChars("b")},
       {factory->NewStringFromStaticChars("a"),
        factory->NewStringFromStaticChars(""),
-       factory->NewStringFromStaticChars("a"),
-       BinaryOperationFeedback::kString},
+       factory->NewStringFromStaticChars("a")},
       {factory->NewStringFromStaticChars("1.11"), factory->NewHeapNumber(2.5),
-       factory->NewStringFromStaticChars("1.112.5"),
-       BinaryOperationFeedback::kAny},
+       factory->NewStringFromStaticChars("1.112.5")},
       {factory->NewStringFromStaticChars("-1.11"), factory->NewHeapNumber(2.56),
-       factory->NewStringFromStaticChars("-1.112.56"),
-       BinaryOperationFeedback::kAny},
+       factory->NewStringFromStaticChars("-1.112.56")},
       {factory->NewStringFromStaticChars(""), factory->NewHeapNumber(2.5),
-       factory->NewStringFromStaticChars("2.5"), BinaryOperationFeedback::kAny},
+       factory->NewStringFromStaticChars("2.5")},
   };
 
   for (size_t i = 0; i < arraysize(test_cases); i++) {
@@ -437,11 +429,6 @@ TEST(InterpreterStringAdd) {
     auto callable = tester.GetCallable<>();
     Handle<Object> return_value = callable().ToHandleChecked();
     CHECK(return_value->SameValue(*test_cases[i].expected_value));
-
-    Object* feedback = vector->Get(slot);
-    CHECK(feedback->IsSmi());
-    CHECK_EQ(test_cases[i].expected_feedback,
-             static_cast<Smi*>(feedback)->value());
   }
 }
 
@@ -1716,7 +1703,7 @@ TEST(InterpreterSmiComparisons) {
                  CompareC(comparison, inputs[i], inputs[j]));
         Object* feedback = vector->Get(slot);
         CHECK(feedback->IsSmi());
-        CHECK_EQ(CompareOperationFeedback::kSignedSmall,
+        CHECK_EQ(BinaryOperationFeedback::kSignedSmall,
                  static_cast<Smi*>(feedback)->value());
       }
     }
@@ -1763,7 +1750,7 @@ TEST(InterpreterHeapNumberComparisons) {
                  CompareC(comparison, inputs[i], inputs[j]));
         Object* feedback = vector->Get(slot);
         CHECK(feedback->IsSmi());
-        CHECK_EQ(CompareOperationFeedback::kNumber,
+        CHECK_EQ(BinaryOperationFeedback::kNumber,
                  static_cast<Smi*>(feedback)->value());
       }
     }
@@ -1809,7 +1796,7 @@ TEST(InterpreterStringComparisons) {
                  CompareC(comparison, inputs[i], inputs[j]));
         Object* feedback = vector->Get(slot);
         CHECK(feedback->IsSmi());
-        CHECK_EQ(CompareOperationFeedback::kAny,
+        CHECK_EQ(BinaryOperationFeedback::kAny,
                  static_cast<Smi*>(feedback)->value());
       }
     }
@@ -1875,7 +1862,7 @@ TEST(InterpreterMixedComparisons) {
                    CompareC(comparison, lhs, rhs, true));
           Object* feedback = vector->Get(slot);
           CHECK(feedback->IsSmi());
-          CHECK_EQ(CompareOperationFeedback::kAny,
+          CHECK_EQ(BinaryOperationFeedback::kAny,
                    static_cast<Smi*>(feedback)->value());
         }
       }