dart2js cps: Refactor and optimize string concatenations.

dart2js cps: Refactor and optimize string concatenations.

The ConcatenateStrings IR node has been removed. It was a leftover from
dart2dart.

Expressions inside string interpolations are now stringified explicitly
and we concatenate strings with a pure StringConcat operator.

ConstantExpressions have also been removed from the IR. They were also
an artifact from dart2dart and were complicating the optimizer.

BUG=
R=kmillikin@google.com

Committed: https://github.com/dart-lang/sdk/commit/ac75893964144d5712d1da8417c27c232750ec37

Reverted: https://github.com/dart-lang/sdk/commit/988b65336b3194cd539781195a9b8be24b3bbdc3

Committed: https://github.com/dart-lang/sdk/commit/04c5c61a1033e5e050d353da5d295d246d26ffcb

[asgerf, 2015-06-18 14:57:54]

asgerf@google.com changed reviewers:
+ karlklose@google.com, kmillikin@google.com

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

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/builtin_operator.dart (right):

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:45: StringConcat,
I would prefer not to abbreviate the enum identifiers - it should not make a big
difference in line length and reads much nicer.

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (left):

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1482:
assert(irBuilder.isOpen);
Shouldn't we keep the assert? (here and in the function below?)

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1482: Iterator<ast.Node> it
= node.parts.iterator;
How about:
  for (ast.StringInterpolationPart part in node.parts) {
    buildStringParts(part.expression, accumulator);
    buildStringParts(part.string, accumulator);
  }

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2074: FunctionElement get
throwTypeErrorHelper  => _backend.getThrowTypeError();
Revert this change.

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:718: values.ConstantValue value;
Is this field mutated now or can we keep it final?

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:679: Primitive arg(int n) =>
node.arguments[n].definition;
Could you inline the two helpers?

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:681: if
(node.target.library.isInternalLibrary) {
Change to one conditional?
  if (node.target.library.isInternalLibrary &&
      node.target.name == InternalMethod.Stringify) {
    ...
  }

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:724: case
BuiltinOperator.StringConcat:
Use if instead?

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:735: DartString
getString(AbstractValue value) {
Consider moving this helper to the top of the method.

I would also get rid of the cast (it must also be checked in production mode)
and add a checked mode check instead.

Perhaps like the following:
  DartString getString(AbstractValue value) {
    StringConstantValue constant = value.constant;
    return constant.primitiveValue;
  }

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:984: switch (node.target.name)
{
Use if?

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1074: case
BuiltinOperator.StringConcat:
Use if?

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1441: static const String
Stringify = 'S';
Make this a predicate to test on the element instead.  Maybe there is already a
field on the compiler that holds the element.

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

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/builtin_operator.dart (right):

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:45: StringConcat,
On 2015/06/19 08:33:44, karlklose wrote:
> I would prefer not to abbreviate the enum identifiers - it should not make a
big
> difference in line length and reads much nicer.

Done.

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (left):

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1482:
assert(irBuilder.isOpen);
On 2015/06/19 08:33:44, karlklose wrote:
> Shouldn't we keep the assert? (here and in the function below?)

Done.

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1482: Iterator<ast.Node> it
= node.parts.iterator;
On 2015/06/19 08:33:45, karlklose wrote:
> How about:
>   for (ast.StringInterpolationPart part in node.parts) {
>     buildStringParts(part.expression, accumulator);
>     buildStringParts(part.string, accumulator);
>   }

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

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2074: FunctionElement get
throwTypeErrorHelper  => _backend.getThrowTypeError();
On 2015/06/19 08:33:45, karlklose wrote:
> Revert this change.

Done.

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:718: values.ConstantValue value;
On 2015/06/19 08:33:45, karlklose wrote:
> Is this field mutated now or can we keep it final?

We can keep it final.

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:679: Primitive arg(int n) =>
node.arguments[n].definition;
On 2015/06/19 08:33:45, karlklose wrote:
> Could you inline the two helpers?

We're going to need them anyway.

Why I am doing this:
If pattern matching on the IR turns out to be hopelessly verbose and infeasible,
we would want to know about it ASAP. Implementing one case as if there is only
ever going to be one case will not reveal any such problems, so I am writing it
in a way that I feel would support having more cases added easily.

It also means it IS actually going to easier to add new cases.

I've added a TODO to clarify that we don't have a single-case switch just for
the heck of it.

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:681: if
(node.target.library.isInternalLibrary) {
On 2015/06/19 08:33:45, karlklose wrote:
> Change to one conditional?
>   if (node.target.library.isInternalLibrary &&
>       node.target.name == InternalMethod.Stringify) {
>     ...
>   }

See previous reply.

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:724: case
BuiltinOperator.StringConcat:
On 2015/06/19 08:33:45, karlklose wrote:
> Use if instead?

Same as previous reply.

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:735: DartString
getString(AbstractValue value) {
On 2015/06/19 08:33:45, karlklose wrote:
> Consider moving this helper to the top of the method.
> 
> I would also get rid of the cast (it must also be checked in production mode)
> and add a checked mode check instead.
> 
> Perhaps like the following:
>   DartString getString(AbstractValue value) {
>     StringConstantValue constant = value.constant;
>     return constant.primitiveValue;
>   }

Done.

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:984: switch (node.target.name)
{
On 2015/06/19 08:33:45, karlklose wrote:
> Use if?

See previous reply.

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1074: case
BuiltinOperator.StringConcat:
On 2015/06/19 08:33:45, karlklose wrote:
> Use if?

Same.

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1441: static const String
Stringify = 'S';
On 2015/06/19 08:33:45, karlklose wrote:
> Make this a predicate to test on the element instead.  Maybe there is already
a
> field on the compiler that holds the element.

Are you proposing this?

isStringify(FunctionElement e) => e.library.isInternalLibrary && e.name == 'S'

I don't get it. Why would a predicate be better than a constant?

Or this?

isStringify(FunctionElement e, JavaScriptBackend backend) {
    return backend.getStringInterpolationHelper() == e;
}

That would look up the name 'S' in the library's environment to see if we get
out the element we are already holding. Instead of just looking at the library
and name of that element.

I would prefer not to introduce computational redundancy in the name of some
encapsulation principle that IMO has utterly failed to work in this compiler.

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

It looks fine to me.  I'm very happy to be rid of the distinction between
constant expressions and constant values here.

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1482: Iterator<ast.Node> it
= node.parts.iterator;
On 2015/06/19 11:04:34, asgerf wrote:
> On 2015/06/19 08:33:45, karlklose wrote:
> > How about:
> >   for (ast.StringInterpolationPart part in node.parts) {
> >     buildStringParts(part.expression, accumulator);
> >     buildStringParts(part.string, accumulator);
> >   }
> 
> I would love to, but NodeList is not an Iterable. I don't know why, so I
didn't
> change it.

But it has .iterator.

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:686: new
InvokeContinuation(cont, <Primitive>[arg(0)]);
I wonder if we can come up with a convenient and safe abstraction to take
ownership of references when replacing subterms.

Here we unbox the Reference, allocate a list, map over it to create a new
Reference, add it to the definition's use list, and remove the old reference.

Instead I wish we had: change the reference's parent pointer to point to the
replacement subterm.

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

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1482: Iterator<ast.Node> it
= node.parts.iterator;
On 2015/06/19 11:36:13, Kevin Millikin (Google) wrote:
> On 2015/06/19 11:04:34, asgerf wrote:
> > On 2015/06/19 08:33:45, karlklose wrote:
> > > How about:
> > >   for (ast.StringInterpolationPart part in node.parts) {
> > >     buildStringParts(part.expression, accumulator);
> > >     buildStringParts(part.string, accumulator);
> > >   }
> > 
> > I would love to, but NodeList is not an Iterable. I don't know why, so I
> didn't
> > change it.
> 
> But it has .iterator.

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

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1195573003/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:686: new
InvokeContinuation(cont, <Primitive>[arg(0)]);
On 2015/06/19 11:36:13, Kevin Millikin (Google) wrote:
> I wonder if we can come up with a convenient and safe abstraction to take
> ownership of references when replacing subterms.
> 
> Here we unbox the Reference, allocate a list, map over it to create a new
> Reference, add it to the definition's use list, and remove the old reference.
> 
> Instead I wish we had: change the reference's parent pointer to point to the
> replacement subterm.

Yeah I don't like it either. I tried adding constructors that take references
directly, but it was neither convenient or safe.

[Kevin Millikin (Google), 2015-06-22 15:39:35]

LGTM.

[asgerf, 2015-06-22 16:08:51]

Committed patchset #3 (id:40001) manually as
ac75893964144d5712d1da8417c27c232750ec37 (presubmit successful).

[asgerf, 2015-06-23 11:05:03]

PTAL at type_propagation.dart. I botched up my offline tests and a couple of
bugs went into the first commit.

Sadly the rebase went into the same patch so the delta-diff is messed up.

[Kevin Millikin (Google), 2015-06-23 11:15:53]

LGTM.

[asgerf, 2015-06-23 11:18:05]

Committed patchset #5 (id:80001) manually as
04c5c61a1033e5e050d353da5d295d246d26ffcb (presubmit successful).