Reland "[maglev] Turn add into stringconcat if string input"

verwaest · V8 LUCI CQ · commit f6be4827a049 · 2025-03-31T02:12:26.000-07:00
... forgot about turbolev Bug: 406843931 Change-Id: I295bb9c44f318995270a436723689386f55be78d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6414389 Reviewed-by: Olivier Flückiger <olivf@chromium.org> Commit-Queue: Olivier Flückiger <olivf@chromium.org> Auto-Submit: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/main@{#99571}
diff --git a/src/builtins/builtins-string.tq b/src/builtins/builtins-string.tq
@@ -39,6 +39,10 @@ transitioning macro ToStringImpl(context: Context, o: JSAny): String {
   unreachable;
 }
 
+transitioning builtin ToStringForAdd(context: Context, o: JSAny): String {
+  return ToStringImpl(context, ToPrimitiveDefault(o));
+}
+
 transitioning builtin ToString(context: Context, o: JSAny): String {
   return ToStringImpl(context, o);
 }
diff --git a/src/compiler/turboshaft/assembler.h b/src/compiler/turboshaft/assembler.h
@@ -3627,6 +3627,20 @@ class TurboshaftAssemblerOpInterface
     return CallBuiltin<typename BuiltinCallDescriptor::ToString>(
         isolate, frame_state, context, {input}, lazy_deopt_on_throw);
   }
+  V<String> CallBuiltin_ToStringConvertSymbol(
+      Isolate* isolate, V<turboshaft::FrameState> frame_state,
+      V<Context> context, V<Object> input,
+      LazyDeoptOnThrow lazy_deopt_on_throw) {
+    return CallBuiltin<typename BuiltinCallDescriptor::ToStringConvertSymbol>(
+        isolate, frame_state, context, {input}, lazy_deopt_on_throw);
+  }
+  V<String> CallBuiltin_ToStringForAdd(Isolate* isolate,
+                                       V<turboshaft::FrameState> frame_state,
+                                       V<Context> context, V<Object> input,
+                                       LazyDeoptOnThrow lazy_deopt_on_throw) {
+    return CallBuiltin<typename BuiltinCallDescriptor::ToStringForAdd>(
+        isolate, frame_state, context, {input}, lazy_deopt_on_throw);
+  }
   V<Number> CallBuiltin_PlainPrimitiveToNumber(Isolate* isolate,
                                                V<PlainPrimitive> input) {
     return CallBuiltin<typename BuiltinCallDescriptor::PlainPrimitiveToNumber>(
diff --git a/src/compiler/turboshaft/builtin-call-descriptors.h b/src/compiler/turboshaft/builtin-call-descriptors.h
@@ -293,6 +293,28 @@ struct BuiltinCallDescriptor {
     static constexpr OpEffects kEffects = base_effects.CanCallAnything();
   };
 
+  struct ToStringConvertSymbol : public Descriptor<ToString> {
+    static constexpr auto kFunction = Builtin::kToStringConvertSymbol;
+    using arguments_t = std::tuple<V<Object>>;
+    using results_t = std::tuple<V<String>>;
+
+    static constexpr bool kNeedsFrameState = true;
+    static constexpr bool kNeedsContext = true;
+    static constexpr Operator::Properties kProperties = Operator::kNoProperties;
+    static constexpr OpEffects kEffects = base_effects.CanCallAnything();
+  };
+
+  struct ToStringForAdd : public Descriptor<ToString> {
+    static constexpr auto kFunction = Builtin::kToStringForAdd;
+    using arguments_t = std::tuple<V<Object>>;
+    using results_t = std::tuple<V<String>>;
+
+    static constexpr bool kNeedsFrameState = true;
+    static constexpr bool kNeedsContext = true;
+    static constexpr Operator::Properties kProperties = Operator::kNoProperties;
+    static constexpr OpEffects kEffects = base_effects.CanCallAnything();
+  };
+
   struct PlainPrimitiveToNumber : public Descriptor<PlainPrimitiveToNumber> {
     static constexpr auto kFunction = Builtin::kPlainPrimitiveToNumber;
     using arguments_t = std::tuple<V<PlainPrimitive>>;
diff --git a/src/compiler/turboshaft/turbolev-graph-builder.cc b/src/compiler/turboshaft/turbolev-graph-builder.cc
@@ -2833,21 +2833,24 @@ class GraphBuildingNodeProcessor {
 
     GOTO_IF(__ ObjectIsString(value), done, V<String>::Cast(value));
 
-    IF_NOT (__ IsSmi(value)) {
-      if (node->mode() == maglev::ToString::ConversionMode::kConvertSymbol) {
-        V<i::Map> map = __ LoadMapField(value);
-        V<Word32> instance_type = __ LoadInstanceTypeField(map);
-        IF (__ Word32Equal(instance_type, SYMBOL_TYPE)) {
-          GOTO(done, __ CallRuntime_SymbolDescriptiveString(
-                         isolate_, frame_state, Map(node->context()),
-                         V<Symbol>::Cast(value), ShouldLazyDeoptOnThrow(node)));
-        }
-      }
-    }
+    switch (node->mode()) {
+      case maglev::ToString::kConvertSymbol:
+        GOTO(done, __ CallBuiltin_ToStringConvertSymbol(
+                       isolate_, frame_state, Map(node->context()), value,
+                       ShouldLazyDeoptOnThrow(node)));
+        break;
+      case maglev::ToString::kForAdd:
+        GOTO(done, __ CallBuiltin_ToStringForAdd(isolate_, frame_state,
+                                                 Map(node->context()), value,
+                                                 ShouldLazyDeoptOnThrow(node)));
+        break;
+      case maglev::ToString::kThrowOnSymbol:
+        GOTO(done, __ CallBuiltin_ToString(isolate_, frame_state,
+                                           Map(node->context()), value,
+                                           ShouldLazyDeoptOnThrow(node)));
 
-    GOTO(done,
-         __ CallBuiltin_ToString(isolate_, frame_state, Map(node->context()),
-                                 value, ShouldLazyDeoptOnThrow(node)));
+        break;
+    }
 
     BIND(done, result);
     SetMap(node, result);
diff --git a/src/maglev/maglev-graph-builder.cc b/src/maglev/maglev-graph-builder.cc
@@ -2837,6 +2837,8 @@ template <Operation kOperation>
 ReduceResult MaglevGraphBuilder::VisitBinaryOperation() {
   FeedbackNexus nexus = FeedbackNexusForOperand(1);
   BinaryOperationHint feedback_hint = nexus.GetBinaryOperationFeedback();
+  ValueNode* left = LoadRegister(0);
+  ValueNode* right = GetAccumulator();
   switch (feedback_hint) {
     case BinaryOperationHint::kNone:
       return EmitUnconditionalDeopt(
@@ -2866,8 +2868,6 @@ ReduceResult MaglevGraphBuilder::VisitBinaryOperation() {
     }
     case BinaryOperationHint::kString:
       if constexpr (kOperation == Operation::kAdd) {
-        ValueNode* left = LoadRegister(0);
-        ValueNode* right = GetAccumulator();
         return BuildStringConcat(left, right);
       }
       break;
@@ -2876,8 +2876,6 @@ ReduceResult MaglevGraphBuilder::VisitBinaryOperation() {
         if (broker()
                 ->dependencies()
                 ->DependOnStringWrapperToPrimitiveProtector()) {
-          ValueNode* left = LoadRegister(0);
-          ValueNode* right = GetAccumulator();
           BuildCheckStringOrStringWrapper(left);
           BuildCheckStringOrStringWrapper(right);
           left = BuildUnwrapStringWrapper(left);
@@ -2892,6 +2890,15 @@ ReduceResult MaglevGraphBuilder::VisitBinaryOperation() {
       // Fallback to generic node.
       break;
   }
+  if constexpr (kOperation == Operation::kAdd) {
+    if (CheckType(left, NodeType::kString)) {
+      right = BuildToString(right, ToString::kForAdd);
+      return BuildStringConcat(left, right);
+    } else if (CheckType(right, NodeType::kString)) {
+      left = BuildToString(left, ToString::kForAdd);
+      return BuildStringConcat(left, right);
+    }
+  }
   BuildGenericBinaryOperationNode<kOperation>();
   return ReduceResult::Done();
 }
diff --git a/src/maglev/maglev-ir.cc b/src/maglev/maglev-ir.cc
@@ -5529,12 +5529,19 @@ void ToString::GenerateCode(MaglevAssembler* masm,
   __ JumpIfSmi(value, &call_builtin, Label::Distance::kNear);
   __ JumpIfString(value, &done, Label::Distance::kNear);
   __ bind(&call_builtin);
-  if (mode() == kConvertSymbol) {
-    __ CallBuiltin<Builtin::kToStringConvertSymbol>(context(),       // context
-                                                    value_input());  // input
-  } else {
-    __ CallBuiltin<Builtin::kToString>(context(),       // context
-                                       value_input());  // input
+  switch (mode()) {
+    case kConvertSymbol:
+      __ CallBuiltin<Builtin::kToStringConvertSymbol>(context(),  // context
+                                                      value_input());  // input
+      break;
+    case kForAdd:
+      __ CallBuiltin<Builtin::kToStringForAdd>(context(),       // context
+                                               value_input());  // input
+      break;
+    case kThrowOnSymbol:
+      __ CallBuiltin<Builtin::kToString>(context(),       // context
+                                         value_input());  // input
+      break;
   }
   masm->DefineExceptionHandlerAndLazyDeoptPoint(this);
   __ bind(&done);
diff --git a/src/maglev/maglev-ir.h b/src/maglev/maglev-ir.h
@@ -5123,7 +5123,7 @@ class ToString : public FixedInputValueNodeT<2, ToString> {
   using Base = FixedInputValueNodeT<2, ToString>;
 
  public:
-  enum ConversionMode { kConvertSymbol, kThrowOnSymbol };
+  enum ConversionMode { kConvertSymbol, kThrowOnSymbol, kForAdd };
   explicit ToString(uint64_t bitfield, ConversionMode mode)
       : Base(ConversionModeBitField::update(bitfield, mode)) {}
 
@@ -5144,7 +5144,7 @@ class ToString : public FixedInputValueNodeT<2, ToString> {
   void PrintParams(std::ostream&, MaglevGraphLabeller*) const {}
 
  private:
-  using ConversionModeBitField = NextBitField<ConversionMode, 1>;
+  using ConversionModeBitField = NextBitField<ConversionMode, 2>;
 };
 
 class NumberToString : public FixedInputValueNodeT<1, NumberToString> {

Original file line number	Diff line number	Diff line change
`@@ -39,6 +39,10 @@ transitioning macro ToStringImpl(context: Context, o: JSAny): String {`
`39`	`39`	`unreachable;`
`40`	`40`	`}`
`41`	`41`
	`42`	`+transitioning builtin ToStringForAdd(context: Context, o: JSAny): String {`
	`43`	`+ return ToStringImpl(context, ToPrimitiveDefault(o));`
	`44`	`+}`
	`45`	`+`
`42`	`46`	`transitioning builtin ToString(context: Context, o: JSAny): String {`
`43`	`47`	`return ToStringImpl(context, o);`
`44`	`48`	`}`