Reland of [interpreter] Add string type feedback to add

src/code-stubs.cc

 // Copyright 2012 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.
 
 #include "src/code-stubs.h"
 
 #include <sstream>
 
 #include "src/ast/ast.h"
 #include "src/bootstrapper.h"
@@ skipping 1079 matching lines @@
       }
     }
 
     assembler->Bind(&if_rhsisnotsmi);
     {
       // Load the map of {rhs}.
       Node* rhs_map = assembler->LoadMap(rhs);
 
       // Check if the {rhs} is a HeapNumber.
       assembler->GotoUnless(assembler->IsHeapNumberMap(rhs_map),
                             &call_add_stub);

[rmcilroy, 2016/10/04 08:05:31]

We also need a check here. My suggestion would be to add checks here which check
the unknown side. The type feedback should be String if one side is a string,
and the other is any of a string, number or SMI. So for here you could add:

 assembler->GotoUnless(assembler->IsHeapNumberMap(rhs_map),
                           &check_rhs_string);

 ... 

 assembler->Bind(&check_rhs_string);
 {
   // We know that lhs is a SMI, if rhs is a string type feedback should be
kString, otherwise it should be kAny.
   Node* rhs_instance_type = assembler->LoadMapInstanceType(rhs_map);
   Node* rhs_is_string = assembler->Int32LessThan(rhs_instance_type,
       assembler->Int32Constant(FIRST_NONSTRING_TYPE)),
   var_type_feedback.Bind(
       assembler->Select(rhs_is_string,
           assembler->Int32Constant(BinaryOperationFeedback::kString),
           assembler->Int32Constant(BinaryOperationFeedback::kNumber));
   assembler->Goto(&call_add_stub);

And removing the "var_type_feedback.Bind(..." from call_add_stub.

[Leszek Swirski, 2016/10/04 08:30:40]

Ah, this patch was only adding string + string = string feedback, I can add
string + number = string feedback too if you want.

 
       var_fadd_lhs.Bind(assembler->SmiToFloat64(lhs));
       var_fadd_rhs.Bind(assembler->LoadHeapNumberValue(rhs));
       assembler->Goto(&do_fadd);
     }
   }
 
   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), &call_add_stub);
+    assembler->GotoUnless(assembler->IsHeapNumberMap(lhs_map), &check_string);
 
     // Check if the {rhs} is Smi.
     Label if_rhsissmi(assembler), if_rhsisnotsmi(assembler);
     assembler->Branch(assembler->WordIsSmi(rhs), &if_rhsissmi, &if_rhsisnotsmi);
 
     assembler->Bind(&if_rhsissmi);
     {
       var_fadd_lhs.Bind(assembler->LoadHeapNumberValue(lhs));
       var_fadd_rhs.Bind(assembler->SmiToFloat64(rhs));
       assembler->Goto(&do_fadd);
     }
 
     assembler->Bind(&if_rhsisnotsmi);
     {
       // Load the map of {rhs}.
       Node* rhs_map = assembler->LoadMap(rhs);
 
       // Check if the {rhs} is a HeapNumber.
       assembler->GotoUnless(assembler->IsHeapNumberMap(rhs_map),
                             &call_add_stub);

[rmcilroy, 2016/10/04 08:05:31]

Same comment here as above - here you know that lhs is a heap number, so again
should only have to check the rhs type (maybe you could have a common branch,
although you would need to reload the rhs_map).

[Leszek Swirski, 2016/10/04 08:30:40]

As above.

 
       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), &call_add_stub);

[rmcilroy, 2016/10/04 08:05:31]

We should still count this as a string if the lhs is a string even if rhs is a
SMI (since the output will be string). Here we know that the lhs is a non-number
heap object, but we don't know anything about the rhs, so the code should be
something like:

assembler->Bind(&check_string);
 {
   // Check if lhs is a String.
   Node* lhs_instance_type = assembler->LoadMapInstanceType(lhs_map);
   assembler->GotoUnless(assembler->Int32LessThan(rhs_instance_type,
       assembler->Int32Constant(FIRST_NONSTRING_TYPE), &has_any_type);

   // Check if rhs is a Smi, HeapNumber or String.
   assembler->GotoIf(assembler->WordIsSmi(rhs), &has_string_type);
   Node* rhs_map = assembler->LoadMap(rhs);
   assembler->GotoIf(assembler->IsHeapNumberMap(rhs_map),
       &has_string_type);
   assembler->Branch(assembler->Int32LessThan(
                                rhs_instance_type,
                                assembler->Int32Constant(FIRST_NONSTRING_TYPE)
       &has_string_type,
       &has_any_type);

  assembler->Bind(&has_string_type);
  {
     var_type_feedback.Bind(
        assembler->Int32Constant(BinaryOperationFeedback::kString));
     assembler->Goto(&call_add_stub);
   }
 ...

[Leszek Swirski, 2016/10/04 08:30:40]

As above.

+
+      Node* lhs_instance_type = assembler->LoadMapInstanceType(lhs_map);
+
+      assembler->GotoUnless(assembler->Int32LessThan(
+                                lhs_instance_type,
+                                assembler->Int32Constant(FIRST_NONSTRING_TYPE)),
+                            &call_add_stub);
+
+      Node* rhs_instance_type = assembler->LoadInstanceType(rhs);
+
+      assembler->GotoUnless(assembler->Int32LessThan(
+                                rhs_instance_type,
+                                assembler->Int32Constant(FIRST_NONSTRING_TYPE)),
+                            &call_add_stub);
+
+      var_type_feedback.Bind(
+          assembler->Int32Constant(BinaryOperationFeedback::kString));
+      Callable callable = CodeFactory::Add(assembler->isolate());
+      var_result.Bind(assembler->CallStub(callable, context, lhs, rhs));

[rmcilroy, 2016/10/04 08:05:31]

Let's keep the calling of the stub and updating of the type feedback in one
place (see comments above).

[Leszek Swirski, 2016/10/04 08:30:40]

Not sure about this, calling the stub is a small amount of code and I think it
might be better to inline it to skip a jump (no pun intended).

+      assembler->Goto(&end);
+    }
   }
 
   assembler->Bind(&do_fadd);
   {
     var_type_feedback.Bind(
         assembler->Int32Constant(BinaryOperationFeedback::kNumber));
     Node* value =
         assembler->Float64Add(var_fadd_lhs.value(), var_fadd_rhs.value());
     Node* result = assembler->ChangeFloat64ToTagged(value);
     var_result.Bind(result);
     assembler->Goto(&end);
   }
 
   assembler->Bind(&call_add_stub);
   {
+    assembler->Comment("Calling the stub");
     var_type_feedback.Bind(
         assembler->Int32Constant(BinaryOperationFeedback::kAny));
     Callable callable = CodeFactory::Add(assembler->isolate());
     var_result.Bind(assembler->CallStub(callable, context, lhs, rhs));
     assembler->Goto(&end);
   }
 
   assembler->Bind(&end);
+  assembler->Comment("Updating the feedback");
   assembler->UpdateFeedback(var_type_feedback.value(), type_feedback_vector,
                             slot_id);
   return var_result.value();
 }
 
 // static
 compiler::Node* SubtractStub::Generate(CodeStubAssembler* assembler,
                                        compiler::Node* left,
                                        compiler::Node* right,
                                        compiler::Node* context) {
@@ skipping 4698 matching lines @@
 
   if (type == MachineType::Pointer()) {
     return Representation::External();
   }
 
   return Representation::Tagged();
 }
 
 }  // namespace internal
 }  // namespace v8