dart2js cps: Refactor and optimize string concatenations.

pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart

Index: pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart
diff --git a/pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart b/pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart
index 3bfeed456686a7f73bb206602e37e4d153e6f6f2..5fa45497dd90d96a2c77119ea8554b8ba9505034 100644
--- a/pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart
+++ b/pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart
@@ -18,6 +18,7 @@ import '../resolution/semantic_visitor.dart';
 import '../resolution/operators.dart' as op;
 import '../tree/tree.dart' as ast;
 import '../universe/universe.dart' show SelectorKind, CallStructure;
+import '../constants/values.dart' show ConstantValue;
 import 'cps_ir_nodes.dart' as ir;
 import 'cps_ir_builder.dart';
 
@@ -342,13 +343,8 @@ abstract class IrBuilderVisitor extends ast.Visitor<ir.Primitive>
   ir.Primitive visitVariableDefinitions(ast.VariableDefinitions node) {
     assert(irBuilder.isOpen);
     if (node.modifiers.isConst) {
-      for (ast.SendSet definition in node.definitions.nodes) {
-        assert(!definition.arguments.isEmpty);
-        assert(definition.arguments.tail.isEmpty);
-        VariableElement element = elements[definition];
-        ConstantExpression value = getConstantForVariable(element);
-        irBuilder.declareLocalConstant(element, value);
-      }
+      // Do nothing.
+      // handleLocalConstantGet inlines the constant at use-site.
     } else {
       for (ast.Node definition in node.definitions.nodes) {
         Element element = elements[definition];
@@ -446,27 +442,24 @@ abstract class IrBuilderVisitor extends ast.Visitor<ir.Primitive>
     return irBuilder.buildDartStringConstant(node.dartString);
   }
 
-  ConstantExpression getConstantForNode(ast.Node node) {
-    ConstantExpression constant =
-        irBuilder.state.constants.getConstantForNode(node, elements);
+  ConstantValue getConstantForNode(ast.Node node) {
+    ConstantValue constant =
+        irBuilder.state.constants.getConstantValueForNode(node, elements);
     assert(invariant(node, constant != null,
         message: 'No constant computed for $node'));
     return constant;
   }
 
-  ConstantExpression getConstantForVariable(VariableElement element) {
-    ConstantExpression constant =
-        irBuilder.state.constants.getConstantForVariable(element);
+  ConstantValue getConstantForVariable(VariableElement element) {
+    ConstantValue constant =
+        irBuilder.state.constants.getConstantValueForVariable(element);
     assert(invariant(element, constant != null,
             message: 'No constant computed for $element'));
     return constant;
   }
 
-  /// Builds a constant pulling the value from the constant environment.
-  // TODO(johnniwinther): Remove this when [IrBuilder.buildConstant] only takes
-  // a [ConstantExpression].
-  ir.Primitive buildConstant(ConstantExpression expression) {
-    return irBuilder.buildConstant(expression,
+  ir.Primitive buildConstantExpression(ConstantExpression expression) {
+    return irBuilder.buildConstant(
         irBuilder.state.constants.getConstantValue(expression));
   }
 
@@ -564,7 +557,7 @@ abstract class IrBuilderVisitor extends ast.Visitor<ir.Primitive>
   ir.Primitive handleConstantGet(
       ast.Node node,
       ConstantExpression constant, _) {
-    return buildConstant(constant);
+    return buildConstantExpression(constant);
   }
 
   /// If [node] is null, returns this.
@@ -601,7 +594,7 @@ abstract class IrBuilderVisitor extends ast.Visitor<ir.Primitive>
       ast.Send node,
       ConstantExpression constant,
       _) {
-    return buildConstant(constant);
+    return buildConstantExpression(constant);
   }
 
   @override
@@ -610,7 +603,7 @@ abstract class IrBuilderVisitor extends ast.Visitor<ir.Primitive>
       LocalVariableElement element,
       _) {
     return element.isConst
-        ? buildConstant(getConstantForVariable(element))
+        ? irBuilder.buildConstant(getConstantForVariable(element))
         : irBuilder.buildLocalVariableGet(element);
   }
 
@@ -920,7 +913,7 @@ abstract class IrBuilderVisitor extends ast.Visitor<ir.Primitive>
       ast.NodeList arguments,
       CallStructure callStructure,
       _) {
-    ir.Primitive target = buildConstant(constant);
+    ir.Primitive target = buildConstantExpression(constant);
     return translateCallInvoke(target, arguments, callStructure);
   }
 
@@ -1257,7 +1250,7 @@ abstract class IrBuilderVisitor extends ast.Visitor<ir.Primitive>
       CompoundRhs rhs,
       arg) {
     return translateCompounds(
-        getValue: () => buildConstant(constant),
+        getValue: () => buildConstantExpression(constant),
         rhs: rhs,
         setValue: (value) {}); // The binary operator will throw before this.
   }
@@ -1478,29 +1471,49 @@ abstract class IrBuilderVisitor extends ast.Visitor<ir.Primitive>
         });
   }
 
+  /// Evaluates a string interpolation and appends each part to [accumulator]
+  /// (after stringify conversion).
+  void buildStringParts(ast.Node node, List<ir.Primitive> accumulator) {
+    if (node is ast.StringJuxtaposition) {
+      buildStringParts(node.first, accumulator);
+      buildStringParts(node.second, accumulator);
+    } else if (node is ast.StringInterpolation) {
+      buildStringParts(node.string, accumulator);
+      Iterator<ast.Node> it = node.parts.iterator;

[karlklose, 2015/06/19 08:33:45]

How about:
  for (ast.StringInterpolationPart part in node.parts) {
    buildStringParts(part.expression, accumulator);
    buildStringParts(part.string, accumulator);
  }

[asgerf, 2015/06/19 11:04:34]

I would love to, but NodeList is not an Iterable. I don't know why, so I didn't
change it.

[Kevin Millikin (Google), 2015/06/19 11:36:13]

But it has .iterator.

[asgerf, 2015/06/19 12:00:52]

Ah, I thought it really had to be an Iterable. Done.

+      while (it.moveNext()) {
+        ast.StringInterpolationPart part = it.current;
+        buildStringParts(part.expression, accumulator);
+        buildStringParts(part.string, accumulator);
+      }
+    } else if (node is ast.LiteralString) {
+      // Empty strings often occur at the end of a string interpolation,
+      // do not bother to include them.
+      if (!node.dartString.isEmpty) {
+        accumulator.add(irBuilder.buildDartStringConstant(node.dartString));
+      }
+    } else if (node is ast.ParenthesizedExpression) {
+      buildStringParts(node, accumulator);
+    } else {
+      ir.Primitive value = visit(node);
+      accumulator.add(irBuilder.buildStringify(value));
+    }
+  }
+
   ir.Primitive visitStringJuxtaposition(ast.StringJuxtaposition node) {
-    assert(irBuilder.isOpen);

[karlklose, 2015/06/19 08:33:44]

Shouldn't we keep the assert? (here and in the function below?)

[asgerf, 2015/06/19 11:04:34]

Done.

-    ir.Primitive first = visit(node.first);
-    ir.Primitive second = visit(node.second);
-    return irBuilder.buildStringConcatenation([first, second]);
+    List<ir.Primitive> parts = <ir.Primitive>[];
+    buildStringParts(node, parts);
+    return irBuilder.buildStringConcatenation(parts);
   }
 
   ir.Primitive visitStringInterpolation(ast.StringInterpolation node) {
-    assert(irBuilder.isOpen);
-    List<ir.Primitive> arguments = [];
-    arguments.add(visitLiteralString(node.string));
-    var it = node.parts.iterator;
-    while (it.moveNext()) {
-      ast.StringInterpolationPart part = it.current;
-      arguments.add(visit(part.expression));
-      arguments.add(visitLiteralString(part.string));
-    }
-    return irBuilder.buildStringConcatenation(arguments);
+    List<ir.Primitive> parts = <ir.Primitive>[];
+    buildStringParts(node, parts);
+    return irBuilder.buildStringConcatenation(parts);
   }
 
   ir.Primitive translateConstant(ast.Node node) {
     assert(irBuilder.isOpen);
-    return buildConstant(getConstantForNode(node));
+    return irBuilder.buildConstant(getConstantForNode(node));
   }
 
   ir.Primitive visitThrow(ast.Throw node) {
@@ -2054,7 +2067,11 @@ class GlobalProgramInformation {
     return cls.typeVariables.isNotEmpty && _backend.classNeedsRti(cls);
   }
 
-  FunctionElement get throwTypeErrorHelper => _backend.getThrowTypeError();
+  FunctionElement get stringifyFunction {
+    return _backend.getStringInterpolationHelper();
+  }
+
+  FunctionElement get throwTypeErrorHelper  => _backend.getThrowTypeError();

[karlklose, 2015/06/19 08:33:45]

Revert this change.

[asgerf, 2015/06/19 11:04:34]

Done.

 
   ClassElement get nullClass => _compiler.nullClass;
 
@@ -2800,10 +2817,9 @@ class JsIrBuilderVisitor extends IrBuilderVisitor {
   }
 
   ir.Primitive buildStaticFieldGet(FieldElement field, SourceInformation src) {
-    ConstantExpression constant =
-        backend.constants.getConstantForVariable(field);
+    ConstantValue constant = getConstantForVariable(field);
     if (constant != null && !field.isAssignable) {
-      return buildConstant(constant);
+      return irBuilder.buildConstant(constant);
     } else if (backend.constants.lazyStatics.contains(field)) {
       return irBuilder.buildStaticFieldLazyGet(field, src);
     } else {