Inline number to string conversion for string addition into BinaryOp(Stub).

src/hydrogen.cc

Index: src/hydrogen.cc
diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 14ac329575680c20e6cbac81e6298a9f6ef914da..239a1aa046915bbd291fffa1acc2583e31571cc4 100644
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -1283,9 +1283,63 @@ void HGraphBuilder::BuildTransitionElementsKind(HValue* object,
 }
 
 
-HValue* HGraphBuilder::BuildLookupNumberStringCache(
-    HValue* object,
-    HIfContinuation* continuation) {
+HValue* HGraphBuilder::BuildSmiToString(HValue* number) {
+  // Everything in here is safe wrt. observable side effects.

[rossberg, 2013/10/17 11:02:19]

Nit: this comment seems redundant, the next line says it all.

[Benedikt Meurer, 2013/10/18 06:53:05]

Done.

+  NoObservableSideEffectsScope scope(this);
+
+  // Load the number string cache.
+  HValue* number_string_cache =
+      Add<HLoadRoot>(Heap::kNumberStringCacheRootIndex);
+
+  // Make the hash maks from the length of the number string cache. It

[rossberg, 2013/10/17 11:02:19]

Typo: mask

[Benedikt Meurer, 2013/10/18 06:53:05]

Done.

+  // contains two elements (number and string) for each cache entry.
+  HValue* mask = AddLoadFixedArrayLength(number_string_cache);
+  mask->set_type(HType::Smi());
+  mask = Add<HSar>(mask, graph()->GetConstant1());
+  mask = Add<HSub>(mask, graph()->GetConstant1());
+
+  // Compute hash for smi similar to smi_get_hash().
+  HValue* hash = Add<HBitwise>(Token::BIT_AND, number, mask);
+
+  // Load the key.
+  HValue* key_index = Add<HShl>(hash, graph()->GetConstant1());
+  HValue* key = Add<HLoadKeyed>(number_string_cache, key_index,
+                                static_cast<HValue*>(NULL),
+                                FAST_ELEMENTS, ALLOW_RETURN_HOLE);
+
+  // Check if number == key.
+  IfBuilder if_numberiskey(this);
+  if_numberiskey.If<HCompareObjectEqAndBranch>(number, key);
+  if_numberiskey.Then();
+  {
+    // Count number to string operation in native code.
+    AddIncrementCounter(isolate()->counters()->number_to_string_native());
+
+    // Load the value in case of cache hit.
+    HValue* value_index = Add<HAdd>(key_index, graph()->GetConstant1());
+    Push(Add<HLoadKeyed>(number_string_cache, value_index,
+                         static_cast<HValue*>(NULL),
+                         FAST_ELEMENTS, ALLOW_RETURN_HOLE));
+  }
+  if_numberiskey.Else();
+  {
+    // Cache miss, fallback to runtime.
+    Add<HPushArgument>(number);
+    Push(Add<HCallRuntime>(
+            isolate()->factory()->empty_string(),
+            Runtime::FunctionForId(Runtime::kNumberToStringSkipCache),
+            1));
+  }
+  if_numberiskey.End();
+
+  return Pop();
+}
+
+
+HValue* HGraphBuilder::BuildNumberToString(HValue* number) {
+  // Everything in here is safe wrt. observable side effects.

[rossberg, 2013/10/17 11:02:19]

Nit: as above

[Benedikt Meurer, 2013/10/18 06:53:05]

Done.

+  NoObservableSideEffectsScope scope(this);
+
   // Create a joinable continuation.
   HIfContinuation found(graph()->CreateBasicBlock(),
                         graph()->CreateBasicBlock());
@@ -1301,13 +1355,13 @@ HValue* HGraphBuilder::BuildLookupNumberStringCache(
   mask = Add<HSar>(mask, graph()->GetConstant1());
   mask = Add<HSub>(mask, graph()->GetConstant1());
 
-  // Check whether object is a smi.
-  IfBuilder if_objectissmi(this);
-  if_objectissmi.If<HIsSmiAndBranch>(object);
-  if_objectissmi.Then();
+  // Check whether number is a smi.
+  IfBuilder if_numberissmi(this);
+  if_numberissmi.If<HIsSmiAndBranch>(number);
+  if_numberissmi.Then();
   {

[oliv, 2013/10/17 10:56:30]

Please find a way to share the code with BuildSmiToString here...

[Benedikt Meurer, 2013/10/18 06:53:05]

Done.

     // Compute hash for smi similar to smi_get_hash().
-    HValue* hash = Add<HBitwise>(Token::BIT_AND, object, mask);
+    HValue* hash = Add<HBitwise>(Token::BIT_AND, number, mask);
 
     // Load the key.
     HValue* key_index = Add<HShl>(hash, graph()->GetConstant1());
@@ -1315,104 +1369,81 @@ HValue* HGraphBuilder::BuildLookupNumberStringCache(
                                   static_cast<HValue*>(NULL),
                                   FAST_ELEMENTS, ALLOW_RETURN_HOLE);
 
-    // Check if object == key.
-    IfBuilder if_objectiskey(this);
-    if_objectiskey.If<HCompareObjectEqAndBranch>(key, object);
-    if_objectiskey.Then();
+    // Check if number == key.
+    IfBuilder if_numberiskey(this);
+    if_numberiskey.If<HCompareObjectEqAndBranch>(number, key);
+    if_numberiskey.Then();
     {
       // Make the key_index available.
       Push(key_index);
     }
-    if_objectiskey.JoinContinuation(&found);
+    if_numberiskey.JoinContinuation(&found);
   }
-  if_objectissmi.Else();
+  if_numberissmi.Else();
   {
-    // Check if object is a heap number.
-    IfBuilder if_objectisnumber(this);
-    if_objectisnumber.If<HCompareMap>(
-        object, isolate()->factory()->heap_number_map());
-    if_objectisnumber.Then();
+    // Check that the number is actually a heap number.
+    BuildCheckMap(number, isolate()->factory()->heap_number_map());
+
+    // Compute hash for heap number similar to double_get_hash().
+    HValue* low = Add<HLoadNamedField>(
+        number, HObjectAccess::ForHeapNumberValueLowestBits());
+    HValue* high = Add<HLoadNamedField>(
+        number, HObjectAccess::ForHeapNumberValueHighestBits());
+    HValue* hash = Add<HBitwise>(Token::BIT_XOR, low, high);
+    hash = Add<HBitwise>(Token::BIT_AND, hash, mask);
+
+    // Load the key.
+    HValue* key_index = Add<HShl>(hash, graph()->GetConstant1());
+    HValue* key = Add<HLoadKeyed>(number_string_cache, key_index,
+                                  static_cast<HValue*>(NULL),
+                                  FAST_ELEMENTS, ALLOW_RETURN_HOLE);
+
+    // Check if key is a heap number (the number string cache contains only
+    // SMIs and heap number, so it is sufficient to do a SMI check here).
+    IfBuilder if_keyisnotsmi(this);
+    if_keyisnotsmi.IfNot<HIsSmiAndBranch>(key);
+    if_keyisnotsmi.Then();
     {
-      // Compute hash for heap number similar to double_get_hash().
-      HValue* low = Add<HLoadNamedField>(
-          object, HObjectAccess::ForHeapNumberValueLowestBits());
-      HValue* high = Add<HLoadNamedField>(
-          object, HObjectAccess::ForHeapNumberValueHighestBits());
-      HValue* hash = Add<HBitwise>(Token::BIT_XOR, low, high);
-      hash = Add<HBitwise>(Token::BIT_AND, hash, mask);
-
-      // Load the key.
-      HValue* key_index = Add<HShl>(hash, graph()->GetConstant1());
-      HValue* key = Add<HLoadKeyed>(number_string_cache, key_index,
-                                    static_cast<HValue*>(NULL),
-                                    FAST_ELEMENTS, ALLOW_RETURN_HOLE);
-
-      // Check if key is a heap number.
-      IfBuilder if_keyisnumber(this);
-      if_keyisnumber.IfNot<HIsSmiAndBranch>(key);
-      if_keyisnumber.AndIf<HCompareMap>(
-          key, isolate()->factory()->heap_number_map());
-      if_keyisnumber.Then();
+      // Check if values of key and object match.
+      IfBuilder if_keyeqnumber(this);
+      if_keyeqnumber.If<HCompareNumericAndBranch>(
+          Add<HLoadNamedField>(key, HObjectAccess::ForHeapNumberValue()),
+          Add<HLoadNamedField>(number, HObjectAccess::ForHeapNumberValue()),
+          Token::EQ);
+      if_keyeqnumber.Then();
       {
-        // Check if values of key and object match.
-        IfBuilder if_keyeqobject(this);
-        if_keyeqobject.If<HCompareNumericAndBranch>(
-            Add<HLoadNamedField>(key, HObjectAccess::ForHeapNumberValue()),
-            Add<HLoadNamedField>(object, HObjectAccess::ForHeapNumberValue()),
-            Token::EQ);
-        if_keyeqobject.Then();
-        {
-          // Make the key_index available.
-          Push(key_index);
-        }
-        if_keyeqobject.JoinContinuation(&found);
+        // Make the key_index available.
+        Push(key_index);
       }
-      if_keyisnumber.JoinContinuation(&found);
+      if_keyeqnumber.JoinContinuation(&found);
     }
-    if_objectisnumber.JoinContinuation(&found);
+    if_keyisnotsmi.JoinContinuation(&found);
   }
-  if_objectissmi.End();
+  if_numberissmi.End();
 
   // Check for cache hit.
   IfBuilder if_found(this, &found);
   if_found.Then();
+  {
+    // Count number to string operation in native code.
+    AddIncrementCounter(isolate()->counters()->number_to_string_native());
 
-  // Load the value in case of cache hit.
-  HValue* key_index = Pop();
-  HValue* value_index = Add<HAdd>(key_index, graph()->GetConstant1());
-  HValue* value = Add<HLoadKeyed>(number_string_cache, value_index,
-                                  static_cast<HValue*>(NULL),
-                                  FAST_ELEMENTS, ALLOW_RETURN_HOLE);
-  AddIncrementCounter(isolate()->counters()->number_to_string_native());
-
-  if_found.CaptureContinuation(continuation);
-
-  // The value is only available in true branch of continuation.
-  return value;
-}
-
-
-HValue* HGraphBuilder::BuildNumberToString(HValue* number) {
-  NoObservableSideEffectsScope scope(this);
-
-  // Lookup the number in the number string cache.
-  HIfContinuation continuation;
-  HValue* value = BuildLookupNumberStringCache(number, &continuation);
-  IfBuilder if_found(this, &continuation);
-  if_found.Then();
-
-  // Cache hit.
-  Push(value);
-
+    // Load the value in case of cache hit.
+    HValue* key_index = Pop();
+    HValue* value_index = Add<HAdd>(key_index, graph()->GetConstant1());
+    Push(Add<HLoadKeyed>(number_string_cache, value_index,
+                         static_cast<HValue*>(NULL),
+                         FAST_ELEMENTS, ALLOW_RETURN_HOLE));
+  }
   if_found.Else();
-
-  // Cache miss, fallback to runtime.
-  Add<HPushArgument>(number);
-  Push(Add<HCallRuntime>(
-          isolate()->factory()->empty_string(),
-          Runtime::FunctionForId(Runtime::kNumberToStringSkipCache),
-          1));
-
+  {
+    // Cache miss, fallback to runtime.
+    Add<HPushArgument>(number);
+    Push(Add<HCallRuntime>(
+            isolate()->factory()->empty_string(),
+            Runtime::FunctionForId(Runtime::kNumberToStringSkipCache),
+            1));
+  }
   if_found.End();
 
   return Pop();
@@ -7850,6 +7881,39 @@ HInstruction* HGraphBuilder::BuildBinaryOperation(
     right_rep = Representation::FromType(right_type);
   }
 
+  // Special case for string addition here.
+  if (op == Token::ADD &&
+      (left_type->Is(Type::String()) ||
+       right_type->Is(Type::String()))) {
+    StringAddFlags flags = STRING_ADD_CHECK_NONE;
+
+    if (left_type->Is(Type::String())) {
+      BuildCheckHeapObject(left);
+      AddInstruction(HCheckInstanceType::NewIsString(left, zone()));
+    } else if (left_type->Is(Type::Smi())) {
+      left = BuildSmiToString(EnforceNumberType(left, left_type));
+    } else if (left_type->Is(Type::Number())) {
+      left = BuildNumberToString(left);
+    } else {
+      ASSERT_EQ(STRING_ADD_CHECK_NONE, flags);
+      flags = STRING_ADD_CHECK_LEFT;
+    }
+
+    if (right_type->Is(Type::String())) {
+      BuildCheckHeapObject(right);
+      AddInstruction(HCheckInstanceType::NewIsString(right, zone()));
+    } else if (right_type->Is(Type::Smi())) {
+      right = BuildSmiToString(EnforceNumberType(right, right_type));
+    } else if (right_type->Is(Type::Number())) {
+      right = BuildNumberToString(right);
+    } else {
+      ASSERT_EQ(STRING_ADD_CHECK_NONE, flags);
+      flags = STRING_ADD_CHECK_RIGHT;
+    }
+
+    return NewUncasted<HStringAdd>(left, right, flags);
+  }
+
   if (binop_stub) {
     left = EnforceNumberType(left, left_type);
     right = EnforceNumberType(right, right_type);
@@ -7859,15 +7923,12 @@ HInstruction* HGraphBuilder::BuildBinaryOperation(
 
   bool is_non_primitive = (left_rep.IsTagged() && !left_rep.IsSmi()) ||
                           (right_rep.IsTagged() && !right_rep.IsSmi());
-  bool is_string_add    = op == Token::ADD &&
-                          (left_type->Is(Type::String()) ||
-                           right_type->Is(Type::String()));
 
   HInstruction* instr = NULL;
   // Only the stub is allowed to call into the runtime, since otherwise we would
   // inline several instructions (including the two pushes) for every tagged
   // operation in optimized code, which is more expensive, than a stub call.
-  if (binop_stub && is_non_primitive && !is_string_add) {
+  if (binop_stub && is_non_primitive) {
     HValue* function = AddLoadJSBuiltin(BinaryOpIC::TokenToJSBuiltin(op));
     Add<HPushArgument>(left);
     Add<HPushArgument>(right);
@@ -7875,23 +7936,7 @@ HInstruction* HGraphBuilder::BuildBinaryOperation(
   } else {
     switch (op) {
       case Token::ADD:
-        if (is_string_add) {
-          StringAddFlags flags = STRING_ADD_CHECK_BOTH;
-          if (left_type->Is(Type::String())) {
-            BuildCheckHeapObject(left);
-            AddInstruction(HCheckInstanceType::NewIsString(left, zone()));
-            flags = STRING_ADD_CHECK_RIGHT;
-          }
-          if (right_type->Is(Type::String())) {
-            BuildCheckHeapObject(right);
-            AddInstruction(HCheckInstanceType::NewIsString(right, zone()));
-            flags = (flags == STRING_ADD_CHECK_BOTH)
-                ? STRING_ADD_CHECK_LEFT : STRING_ADD_CHECK_NONE;
-          }
-          instr = NewUncasted<HStringAdd>(left, right, flags);
-        } else {
-          instr = NewUncasted<HAdd>(left, right);
-        }
+        instr = NewUncasted<HAdd>(left, right);
         break;
       case Token::SUB:
         instr = NewUncasted<HSub>(left, right);