[turbofan] Introduce a CheckStringAdd node instead of cons string lowering

The new node is introduced for literal string addition and calling
String.prototype.concat in the typed lowering phase. It later might get optimized
away during redundancy elimination, keeping the performance of already existing
benchmarks with string addition. In case the operation is about to throw
(due to too long string being constructed) we just deoptimize, reusing
the interpreter logic for creating the error.

Modify relevant mjsunit and unit tests for string concatenation.

Bug: v8:7902
Change-Id: Ie97d39534df4480fa8d4fe3ba276d02ed5e750e3
Reviewed-on: https://chromium-review.googlesource.com/1193342
Commit-Queue: Maya Lekova <mslekova@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55482}

src/compiler/effect-control-linearizer.cc

@@ -694,6 +694,9 @@ bool EffectControlLinearizer::TryWireInStateEffect(Node* node,
     case IrOpcode::kCheckIf:
       LowerCheckIf(node, frame_state);
       break;
+    case IrOpcode::kCheckStringAdd:
+      result = LowerCheckStringAdd(node, frame_state);
+      break;
     case IrOpcode::kCheckedInt32Add:
       result = LowerCheckedInt32Add(node, frame_state);
       break;
@@ -1544,6 +1547,32 @@ void EffectControlLinearizer::LowerCheckIf(Node* node, Node* frame_state) {
   __ DeoptimizeIfNot(p.reason(), p.feedback(), value, frame_state);
 }
 
+Node* EffectControlLinearizer::LowerCheckStringAdd(Node* node,
+                                                   Node* frame_state) {
+  Node* lhs = node->InputAt(0);
+  Node* rhs = node->InputAt(1);
+
+  Node* lhs_length = __ LoadField(AccessBuilder::ForStringLength(), lhs);
+  Node* rhs_length = __ LoadField(AccessBuilder::ForStringLength(), rhs);
+  Node* check = __ IntLessThan(__ IntAdd(lhs_length, rhs_length),
+                               __ SmiConstant(String::kMaxLength));
+
+  __ DeoptimizeIfNot(DeoptimizeReason::kOverflow, VectorSlotPair(), check,
+                     frame_state);
+
+  Callable const callable =
+      CodeFactory::StringAdd(isolate(), STRING_ADD_CHECK_NONE, NOT_TENURED);
+  auto call_descriptor = Linkage::GetStubCallDescriptor(
+      graph()->zone(), callable.descriptor(), 0, CallDescriptor::kNoFlags,
+      Operator::kNoDeopt | Operator::kNoWrite | Operator::kNoThrow);
+
+  Node* value =
+      __ Call(call_descriptor, jsgraph()->HeapConstant(callable.code()), lhs,
+              rhs, __ NoContextConstant());
+
+  return value;
+}
+
 Node* EffectControlLinearizer::LowerCheckedInt32Add(Node* node,
                                                     Node* frame_state) {
   Node* lhs = node->InputAt(0);

src/compiler/effect-control-linearizer.h

@@ -67,6 +67,7 @@ class V8_EXPORT_PRIVATE EffectControlLinearizer {
   Node* LowerCheckString(Node* node, Node* frame_state);
   Node* LowerCheckSymbol(Node* node, Node* frame_state);
   void LowerCheckIf(Node* node, Node* frame_state);
+  Node* LowerCheckStringAdd(Node* node, Node* frame_state);
   Node* LowerCheckedInt32Add(Node* node, Node* frame_state);
   Node* LowerCheckedInt32Sub(Node* node, Node* frame_state);
   Node* LowerCheckedInt32Div(Node* node, Node* frame_state);

src/compiler/js-call-reducer.cc

@@ -5454,7 +5454,6 @@ Reduction JSCallReducer::ReduceStringPrototypeConcat(
   }
   Node* effect = NodeProperties::GetEffectInput(node);
   Node* control = NodeProperties::GetControlInput(node);
-  Node* context = NodeProperties::GetContextInput(node);
   Node* receiver = effect =
       graph()->NewNode(simplified()->CheckString(p.feedback()),
                        NodeProperties::GetValueInput(node, 1), effect, control);
@@ -5463,26 +5462,17 @@ Reduction JSCallReducer::ReduceStringPrototypeConcat(
     ReplaceWithValue(node, receiver, effect, control);
     return Replace(receiver);
   }
+
+  if (!isolate()->IsStringLengthOverflowIntact()) {
+    return NoChange();
+  }
+
   Node* argument = effect =
       graph()->NewNode(simplified()->CheckString(p.feedback()),
                        NodeProperties::GetValueInput(node, 2), effect, control);
 
-  Callable const callable =
-      CodeFactory::StringAdd(isolate(), STRING_ADD_CHECK_NONE, NOT_TENURED);
-  auto call_descriptor =
-      Linkage::GetStubCallDescriptor(graph()->zone(), callable.descriptor(), 0,
-                                     CallDescriptor::kNeedsFrameState,
-                                     Operator::kNoDeopt | Operator::kNoWrite);
-
-  // TODO(turbofan): Massage the FrameState of the {node} here once we
-  // have an artificial builtin frame type, so that it looks like the
-  // exception from StringAdd overflow came from String.prototype.concat
-  // builtin instead of the calling function.
-  Node* outer_frame_state = NodeProperties::GetFrameStateInput(node);
-
-  Node* value = effect = control = graph()->NewNode(
-      common()->Call(call_descriptor), jsgraph()->HeapConstant(callable.code()),
-      receiver, argument, context, outer_frame_state, effect, control);
+  Node* value = effect = graph()->NewNode(simplified()->CheckStringAdd(),
+                                          receiver, argument, effect, control);
 
   ReplaceWithValue(node, value, effect, control);
   return Replace(value);

src/compiler/js-typed-lowering.cc

@@ -530,41 +530,15 @@ Reduction JSTypedLowering::ReduceJSAdd(Node* node) {
         NodeProperties::ReplaceValueInput(node, reduction.replacement(), 0);
       }
     }
-    // We might be able to constant-fold the String concatenation now.
-    if (r.BothInputsAre(Type::String())) {
-      HeapObjectBinopMatcher m(node);
-      if (m.IsFoldable()) {
-        StringRef left = m.left().Ref(js_heap_broker()).AsString();
-        StringRef right = m.right().Ref(js_heap_broker()).AsString();
-        if (left.length() + right.length() > String::kMaxLength) {
-          // No point in trying to optimize this, as it will just throw.
-          return NoChange();
-        }
-        // TODO(mslekova): get rid of these allows by doing either one of:
-        // 1. remove the optimization and check if it ruins the performance
-        // 2. leave a placeholder and do the actual allocations once back on the
-        // MT
-        AllowHandleDereference allow_handle_dereference;
-        AllowHandleAllocation allow_handle_allocation;
-        AllowHeapAllocation allow_heap_allocation;
-        ObjectRef cons(
-            js_heap_broker(),
-            factory()
-                ->NewConsString(left.object<String>(), right.object<String>())
-                .ToHandleChecked());
-        Node* value = jsgraph()->Constant(cons);
-        ReplaceWithValue(node, value);
-        return Replace(value);
-      }
-    }
     // We might know for sure that we're creating a ConsString here.
     if (r.ShouldCreateConsString()) {
       return ReduceCreateConsString(node);
     }
-    // Eliminate useless concatenation of empty string.
     if (r.BothInputsAre(Type::String())) {
       Node* effect = NodeProperties::GetEffectInput(node);
       Node* control = NodeProperties::GetControlInput(node);
+
+      // Eliminate useless concatenation of empty string.
       if (r.LeftInputIs(empty_string_type_)) {
         Node* value = effect =
             graph()->NewNode(simplified()->CheckString(VectorSlotPair()),
@@ -578,7 +552,18 @@ Reduction JSTypedLowering::ReduceJSAdd(Node* node) {
         ReplaceWithValue(node, value, effect, control);
         return Replace(value);
       }
+
+      // TODO(mslekova): Make sure this is executed for cons string
+      // as well.
+      if (isolate()->IsStringLengthOverflowIntact()) {
+        Node* value = graph()->NewNode(simplified()->CheckStringAdd(), r.left(),
+                                       r.right(), effect, control);
+
+        ReplaceWithValue(node, value, value);
+        return Replace(value);
+      }
     }
+
     StringAddFlags flags = STRING_ADD_CHECK_NONE;
     if (!r.LeftInputIs(Type::String())) {
       flags = STRING_ADD_CONVERT_LEFT;
@@ -592,6 +577,7 @@ Reduction JSTypedLowering::ReduceJSAdd(Node* node) {
       // effects; it can still throw obviously.
       properties = Operator::kNoWrite | Operator::kNoDeopt;
     }
+
     // JSAdd(x:string, y) => CallStub[StringAdd](x, y)
     // JSAdd(x, y:string) => CallStub[StringAdd](x, y)
     Callable const callable =

src/compiler/opcodes.h

@@ -372,6 +372,7 @@
   V(CheckNotTaggedHole)                 \
   V(CheckEqualsInternalizedString)      \
   V(CheckEqualsSymbol)                  \
+  V(CheckStringAdd)                     \
   V(CompareMaps)                        \
   V(ConvertReceiver)                    \
   V(ConvertTaggedHoleToUndefined)       \

src/compiler/redundancy-elimination.cc

@@ -32,6 +32,7 @@ Reduction RedundancyElimination::Reduce(Node* node) {
     case IrOpcode::kCheckSmi:
     case IrOpcode::kCheckString:
     case IrOpcode::kCheckSymbol:
+    case IrOpcode::kCheckStringAdd:
     case IrOpcode::kCheckedFloat64ToInt32:
     case IrOpcode::kCheckedInt32Add:
     case IrOpcode::kCheckedInt32Div:

src/compiler/simplified-lowering.cc

@@ -3140,6 +3140,7 @@ class RepresentationSelector {
       case IrOpcode::kArgumentsLengthState:
       case IrOpcode::kUnreachable:
       case IrOpcode::kRuntimeAbort:
+      case IrOpcode::kCheckStringAdd:
 // All JavaScript operators except JSToNumber have uniform handling.
 #define OPCODE_CASE(name) case IrOpcode::k##name:
         JS_SIMPLE_BINOP_LIST(OPCODE_CASE)

src/compiler/simplified-operator.cc

@@ -790,7 +790,8 @@ bool operator==(CheckMinusZeroParameters const& lhs,
   V(CheckedInt32Mod, 2, 1)               \
   V(CheckedInt32Sub, 2, 1)               \
   V(CheckedUint32Div, 2, 1)              \
-  V(CheckedUint32Mod, 2, 1)
+  V(CheckedUint32Mod, 2, 1)              \
+  V(CheckStringAdd, 2, 1)
 
 #define CHECKED_WITH_FEEDBACK_OP_LIST(V) \
   V(CheckBounds, 2, 1)                   \

src/compiler/simplified-operator.h

@@ -678,6 +678,8 @@ class V8_EXPORT_PRIVATE SimplifiedOperatorBuilder final
   const Operator* CheckString(const VectorSlotPair& feedback);
   const Operator* CheckSymbol();
 
+  const Operator* CheckStringAdd();
+
   const Operator* CheckedFloat64ToInt32(CheckForMinusZeroMode,
                                         const VectorSlotPair& feedback);
   const Operator* CheckedInt32Add();

src/compiler/typer.cc

@@ -2001,6 +2001,8 @@ Type Typer::Visitor::TypeCheckSymbol(Node* node) {
   return Type::Intersect(arg, Type::Symbol(), zone());
 }
 
+Type Typer::Visitor::TypeCheckStringAdd(Node* node) { return Type::String(); }
+
 Type Typer::Visitor::TypeCheckFloat64Hole(Node* node) {
   return typer_->operation_typer_.CheckFloat64Hole(Operand(node, 0));
 }

src/compiler/verifier.cc

@@ -1430,7 +1430,11 @@ void Verifier::Visitor::Check(Node* node, const AllNodes& all) {
       CheckValueInputIs(node, 0, Type::Any());
       CheckTypeIs(node, Type::Symbol());
       break;
-
+    case IrOpcode::kCheckStringAdd:
+      CheckValueInputIs(node, 0, Type::String());
+      CheckValueInputIs(node, 1, Type::String());
+      CheckTypeIs(node, Type::String());
+      break;
     case IrOpcode::kConvertReceiver:
       // (Any, Any) -> Receiver
       CheckValueInputIs(node, 0, Type::Any());

test/js-perf-test/TurboFan/typedLowering.js

@@ -7,7 +7,9 @@ function NumberToString() {
   var num = 10240;
   var obj = {};
 
-  for ( var i = 0; i < num; i++ )
+  for ( var i = 0; i < num; i++ ) {
     ret = obj["test" + num];
+  }
 }
+
 createSuite('NumberToString', 1000, NumberToString);

test/mjsunit/compiler/string-add-try-catch.js

@@ -4,6 +4,9 @@
 
 // Flags: --allow-natives-syntax
 
+// Test that string concatenation overflow (going over string max length)
+// is handled gracefully, i.e. an error is thrown
+
 var a = "a".repeat(%StringMaxLength());
 
 (function() {
@@ -37,3 +40,57 @@ var a = "a".repeat(%StringMaxLength());
   foo("a");
   assertInstanceof(foo(a), RangeError);
 })();
+
+(function() {
+  function foo(a, b) {
+    try {
+      return "0123456789012".concat(a);
+    } catch (e) {
+      return e;
+    }
+  }
+
+  foo("a");
+  foo("a");
+  %OptimizeFunctionOnNextCall(foo);
+  foo("a");
+  assertInstanceof(foo(a), RangeError);
+})();
+
+var obj = {
+  toString: function() {
+    throw new Error('toString has thrown');
+  }
+};
+
+(function() {
+  function foo(a, b) {
+    try {
+      return "0123456789012" + obj;
+    } catch (e) {
+      return e;
+    }
+  }
+
+  foo("a");
+  foo("a");
+  %OptimizeFunctionOnNextCall(foo);
+  foo("a");
+  assertInstanceof(foo(a), Error);
+})();
+
+(function() {
+  function foo(a, b) {
+    try {
+      return a + 123;
+    } catch (e) {
+      return e;
+    }
+  }
+
+  foo("a");
+  foo("a");
+  %OptimizeFunctionOnNextCall(foo);
+  foo("a");
+  assertInstanceof(foo(a), RangeError);
+})();

test/unittests/compiler/js-typed-lowering-unittest.cc

@@ -401,12 +401,7 @@ TEST_F(JSTypedLoweringTest, JSAddWithString) {
   Reduction r = Reduce(graph()->NewNode(javascript()->Add(hint), lhs, rhs,
                                         context, frame_state, effect, control));
   ASSERT_TRUE(r.Changed());
-  EXPECT_THAT(r.replacement(),
-              IsCall(_, IsHeapConstant(
-                            CodeFactory::StringAdd(
-                                isolate(), STRING_ADD_CHECK_NONE, NOT_TENURED)
-                                .code()),
-                     lhs, rhs, context, frame_state, effect, control));
+  EXPECT_THAT(r.replacement(), IsCheckStringAdd(lhs, rhs));
 }
 
 }  // namespace compiler

test/unittests/compiler/node-test-utils.cc

@@ -2126,6 +2126,7 @@ IS_BINOP_MATCHER(Float64Sub)
 IS_BINOP_MATCHER(Float64Mul)
 IS_BINOP_MATCHER(Float64InsertLowWord32)
 IS_BINOP_MATCHER(Float64InsertHighWord32)
+IS_BINOP_MATCHER(CheckStringAdd)
 #undef IS_BINOP_MATCHER
 
 

test/unittests/compiler/node-test-utils.h

@@ -496,6 +496,9 @@ Matcher<Node*> IsStackSlot();
 
 Matcher<Node*> IsSpeculativeToNumber(const Matcher<Node*>& value_matcher);
 
+Matcher<Node*> IsCheckStringAdd(const Matcher<Node*>& lhs_matcher,
+                                const Matcher<Node*>& rhs_matcher);
+
 // Helpers
 static inline Matcher<Node*> IsIntPtrConstant(const intptr_t value) {
   return kPointerSize == 8 ? IsInt64Constant(static_cast<int64_t>(value))