[Interpreter] Collect feedback about Oddballs in Subtract Stub.

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 818 matching lines @@
 
   // Shared entry for floating point subtraction.
   Label do_fsub(assembler), end(assembler),
       call_subtract_stub(assembler, Label::kDeferred);
   Variable var_fsub_lhs(assembler, MachineRepresentation::kFloat64),
       var_fsub_rhs(assembler, MachineRepresentation::kFloat64),
       var_type_feedback(assembler, MachineRepresentation::kWord32),
       var_result(assembler, MachineRepresentation::kTagged);
 
   // Check if the {lhs} is a Smi or a HeapObject.
-  Label if_lhsissmi(assembler), if_lhsisnotsmi(assembler);
+  Label if_lhsissmi(assembler), if_lhsisnotsmi(assembler),
+      if_lhsisnotnumber(assembler), check_rhsisoddball(assembler),

[Leszek Swirski, 2016/10/10 11:57:22]

move the new label definitions to the above label definition list (the one with
do_fsub etc) so that these once are only the ones for the branch.

[mythria, 2016/10/10 13:26:00]

Thanks, Done.

+      call_with_any_feedback(assembler);
   assembler->Branch(assembler->WordIsSmi(lhs), &if_lhsissmi, &if_lhsisnotsmi);
 
   assembler->Bind(&if_lhsissmi);
   {
     // Check if the {rhs} is also a Smi.
     Label if_rhsissmi(assembler), if_rhsisnotsmi(assembler);
     assembler->Branch(assembler->WordIsSmi(rhs), &if_rhsissmi, &if_rhsisnotsmi);
 
     assembler->Bind(&if_rhsissmi);
     {
@@ skipping 22 matching lines @@
       assembler->Goto(&end);
     }
 
     assembler->Bind(&if_rhsisnotsmi);
     {
       // Load the map of the {rhs}.
       Node* rhs_map = assembler->LoadMap(rhs);
 
       // Check if {rhs} is a HeapNumber.
       assembler->GotoUnless(assembler->IsHeapNumberMap(rhs_map),
-                            &call_subtract_stub);
+                            &check_rhsisoddball);
 
       // Perform a floating point subtraction.
       var_fsub_lhs.Bind(assembler->SmiToFloat64(lhs));
       var_fsub_rhs.Bind(assembler->LoadHeapNumberValue(rhs));
       assembler->Goto(&do_fsub);
     }
   }
 
   assembler->Bind(&if_lhsisnotsmi);
   {
     // Load the map of the {lhs}.
     Node* lhs_map = assembler->LoadMap(lhs);
 
     // Check if the {lhs} is a HeapNumber.
     assembler->GotoUnless(assembler->IsHeapNumberMap(lhs_map),
-                          &call_subtract_stub);
+                          &if_lhsisnotnumber);
 
     // Check if the {rhs} is a Smi.
     Label if_rhsissmi(assembler), if_rhsisnotsmi(assembler);
     assembler->Branch(assembler->WordIsSmi(rhs), &if_rhsissmi, &if_rhsisnotsmi);
 
     assembler->Bind(&if_rhsissmi);
     {
       // Perform a floating point subtraction.
       var_fsub_lhs.Bind(assembler->LoadHeapNumberValue(lhs));
       var_fsub_rhs.Bind(assembler->SmiToFloat64(rhs));
       assembler->Goto(&do_fsub);
     }
 
     assembler->Bind(&if_rhsisnotsmi);
     {
       // Load the map of the {rhs}.
       Node* rhs_map = assembler->LoadMap(rhs);
 
       // Check if the {rhs} is a HeapNumber.
       assembler->GotoUnless(assembler->IsHeapNumberMap(rhs_map),
-                            &call_subtract_stub);
+                            &check_rhsisoddball);
 
       // Perform a floating point subtraction.
       var_fsub_lhs.Bind(assembler->LoadHeapNumberValue(lhs));
       var_fsub_rhs.Bind(assembler->LoadHeapNumberValue(rhs));
       assembler->Goto(&do_fsub);
     }
   }
 
   assembler->Bind(&do_fsub);
   {
     var_type_feedback.Bind(
         assembler->Int32Constant(BinaryOperationFeedback::kNumber));
     Node* lhs_value = var_fsub_lhs.value();
     Node* rhs_value = var_fsub_rhs.value();
     Node* value = assembler->Float64Sub(lhs_value, rhs_value);
     var_result.Bind(assembler->ChangeFloat64ToTagged(value));
     assembler->Goto(&end);
   }
 
+  assembler->Bind(&if_lhsisnotnumber);
+  {
+    // No checks on rhs are done yet. We just know lhs is not number or Smi.
+    Label if_rhsissmi(assembler), if_rhsisnotsmi(assembler);

[Leszek Swirski, 2016/10/10 11:57:22]

nit: move these declarations to be next to the branch (to match the surrounding
code style)

[mythria, 2016/10/10 13:26:00]

Done.

+
+    // Check if lhs is an oddball.
+    Node* lhs_instance_type = assembler->LoadInstanceType(lhs);
+    Node* lhs_is_oddball = assembler->Word32Equal(
+        lhs_instance_type, assembler->Int32Constant(ODDBALL_TYPE));
+    assembler->GotoUnless(lhs_is_oddball, &call_with_any_feedback);
+
+    assembler->Branch(assembler->WordIsSmi(rhs), &if_rhsissmi, &if_rhsisnotsmi);
+
+    assembler->Bind(&if_rhsissmi);
+    {
+      var_type_feedback.Bind(
+          assembler->Int32Constant(BinaryOperationFeedback::kNumberOrOddball));
+      assembler->Goto(&call_subtract_stub);

[Leszek Swirski, 2016/10/10 11:57:22]

if lhs is an oddball, can we not extract its float value and jump to the fsub
(with different type feedback of course) rather than calling the stub?

[mythria, 2016/10/10 13:26:00]

As discussed offline, we cannot directly go to fsub. We could still load the
to_number value to save few checks in the subtractStub, but that would involve
additional checks in the check_rhsisoddball case. Assuming Oddballs are not very
frequent for binary operations, this may not give us a performance win as well.
Similar to what we noticed when string add is inlined to Add stub.

+    }
+
+    assembler->Bind(&if_rhsisnotsmi);
+    {
+      // Load the map of the {rhs}.
+      Node* rhs_map = assembler->LoadMap(rhs);
+
+      // Check if {rhs} is a HeapNumber.
+      assembler->GotoUnless(assembler->IsHeapNumberMap(rhs_map),
+                            &check_rhsisoddball);
+
+      var_type_feedback.Bind(
+          assembler->Int32Constant(BinaryOperationFeedback::kNumberOrOddball));
+      assembler->Goto(&call_subtract_stub);

[Leszek Swirski, 2016/10/10 11:57:22]

as above

[mythria, 2016/10/10 13:26:00]

reply above.

+    }
+  }
+
+  assembler->Bind(&check_rhsisoddball);
+  {
+    // Check if rhs is an oddball. At this point we know lhs is either a
+    // Smi or number or oddball and rhs is not a number or Smi.
+    Node* rhs_instance_type = assembler->LoadInstanceType(rhs);
+    Node* rhs_is_oddball = assembler->Word32Equal(
+        rhs_instance_type, assembler->Int32Constant(ODDBALL_TYPE));
+    assembler->GotoUnless(rhs_is_oddball, &call_with_any_feedback);
+
+    var_type_feedback.Bind(
+        assembler->Int32Constant(BinaryOperationFeedback::kNumberOrOddball));
+    assembler->Goto(&call_subtract_stub);

[Leszek Swirski, 2016/10/10 11:57:22]

as above

[mythria, 2016/10/10 13:26:00]

reply above

+  }
+
+  assembler->Bind(&call_with_any_feedback);
+  {
+    var_type_feedback.Bind(
+        assembler->Int32Constant(BinaryOperationFeedback::kAny));
+    assembler->Goto(&call_subtract_stub);
+  }
+
   assembler->Bind(&call_subtract_stub);
   {
     var_type_feedback.Bind(
         assembler->Int32Constant(BinaryOperationFeedback::kAny));

[Leszek Swirski, 2016/10/10 11:57:22]

this needs to be removed

[mythria, 2016/10/10 13:26:00]

oops. I missed it. Thanks, we might have wasted time investigating performance
regressions. Done.

     Callable callable = CodeFactory::Subtract(assembler->isolate());
     var_result.Bind(assembler->CallStub(callable, context, lhs, rhs));
     assembler->Goto(&end);
   }
 
   assembler->Bind(&end);
   assembler->UpdateFeedback(var_type_feedback.value(), type_feedback_vector,
                             slot_id);
   return var_result.value();
 }
@@ skipping 2055 matching lines @@
 
   if (type == MachineType::Pointer()) {
     return Representation::External();
   }
 
   return Representation::Tagged();
 }
 
 }  // namespace internal
 }  // namespace v8