Store-forwarding for ReadTypeVariable

Store-forwarding for ReadTypeVariable

Example 1:

    var v0 = H.setRuntimeTypeInfo(new P.ListQueue(null, 0, 0, 0), [$E]);
    v0._table = J.JSArray_JSArray$list(8, H.getTypeArgumentByIndex(v0, 0));
-->
    var v0 = H.setRuntimeTypeInfo(new P.ListQueue(null, 0, 0, 0), [$E]);
    v0._table = J.JSArray_JSArray$list(8, $E);

Example 2 (enabling scalar replacement):

    this._historyTracking = W._EventStreamSubscription$(
        (v0 = H.setRuntimeTypeInfo(
              new W._EventStream(window, "popstate", false), [null]))._target,
        v0._eventType,
        handler,
        v0._useCapture,
        H.getTypeArgumentByIndex(v0, 0));
-->
    this._historyTracking = W._EventStreamSubscription$(
        window,
        "popstate",
        handler,
        false,
        null);

R=asgerf@google.com

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

[sra1, 2016-01-26 23:45:14]

sra@google.com changed reviewers:
+ asgerf@google.com

[sra1, 2016-01-26 23:45:14]

This is a 26 line change on top of https://codereview.chromium.org/1637843002/

The new code is all in type_propagation.dart

[sra1, 2016-01-27 01:43:12]

Patchset #2 (id:20001) has been deleted

[sra1, 2016-01-27 01:43:21]

Patchset #1 (id:1) has been deleted

[asgerf, 2016-01-27 16:08:01]

lgtm

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

https://codereview.chromium.org/1636193002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2295: context)) {
The bailout should not be necessary because we know the exact class being
created by CreateInstance.  I'm fine with leaving a TODO for now.

[sra1, 2016-01-27 17:58:22]

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

https://codereview.chromium.org/1636193002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2295: context)) {
On 2016/01/27 16:08:01, asgerf wrote:
> The bailout should not be necessary because we know the exact class being
> created by CreateInstance.  I'm fine with leaving a TODO for now.


    class Base<T> { foo() => new Set<T>(); }
    class A<U> extends Base<List<U>> {
      bar() => foo();
    }

if foo() is inlined into bar, the type argument to "new Set",
ReadTypeVariable(this, Base.T) evaluates to List<U>, which is not present in the
vector [U], i.e. there is no valid index.

Given the combination of the concrete instance type and the type variable there
are many more cases we could get (when the vector is rearranged), but not *all*.

We can also read a type variable from a class that has no type arguments, e.g.

    class CodeUnits extends UnmodifiableListBase<int> {}

All the ListMixin code is poisoned by this, e.g. ListMixin.where needs to handle
the CodeUnits case in "new WhereIterable<T>(...)". When this is inlined, we
could do much better.

[asgerf, 2016-01-27 18:23:49]

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

https://codereview.chromium.org/1636193002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2295: context)) {
On 2016/01/27 17:58:22, sra1 wrote:
> On 2016/01/27 16:08:01, asgerf wrote:
> > The bailout should not be necessary because we know the exact class being
> > created by CreateInstance.  I'm fine with leaving a TODO for now.
> 
> 
>     class Base<T> { foo() => new Set<T>(); }
>     class A<U> extends Base<List<U>> {
>       bar() => foo();
>     }
> 
> if foo() is inlined into bar, the type argument to "new Set",
> ReadTypeVariable(this, Base.T) evaluates to List<U>, which is not present in
the
> vector [U], i.e. there is no valid index.
> 
> Given the combination of the concrete instance type and the type variable
there
> are many more cases we could get (when the vector is rearranged), but not
*all*.
> 
> We can also read a type variable from a class that has no type arguments, e.g.
> 
>     class CodeUnits extends UnmodifiableListBase<int> {}
> 
> All the ListMixin code is poisoned by this, e.g. ListMixin.where needs to
handle
> the CodeUnits case in "new WhereIterable<T>(...)". When this is inlined, we
> could do much better.

In both cases we can create a TypeExpression to get List<U> or int. Something
like this:

var type = createdClass.thisType
  .asInstanceOf(typeVariable.enclosingClass)
  .typeArguments[typeVariableIndex];
List args = [];
type.forEeachTypeVariable((v) {
  int index = createdClass.typeParameters.indexOf(v)
  args.add(typeExpression.arguments[index].definition)
})
new TypeExpression(type, args)


The main reason I would not put it here, is that the benefit is not clear when
the resulting type is large, and we don't know for sure if it will lead to
scalar replacement. It's probably best to do it in scalar replacement.

It makes sense to have both I guess, since the optimization here is always a
strict code reduction, even without scalar replacement.

The comment talks about scalar elimination and store forwarding, so it's easy to
get the idea that the bailout is there because the indirection cannot be
eliminated.

[sra1, 2016-01-27 23:46:52]

Description was changed from

==========
Store-forwarding for ReadTypeVariable

Example 1:

    var v0 = H.setRuntimeTypeInfo(new P.ListQueue(null, 0, 0, 0), [$E]);
    v0._table = J.JSArray_JSArray$list(8, H.getTypeArgumentByIndex(v0, 0));
-->
    var v0 = H.setRuntimeTypeInfo(new P.ListQueue(null, 0, 0, 0), [$E]);
    v0._table = J.JSArray_JSArray$list(8, $E);

Example 2 (enabling scalar replacement):

    this._historyTracking = W._EventStreamSubscription$(
        (v0 = H.setRuntimeTypeInfo(
              new W._EventStream(window, "popstate", false), [null]))._target,
        v0._eventType,
        handler,
        v0._useCapture,
        H.getTypeArgumentByIndex(v0, 0));
-->
    this._historyTracking = W._EventStreamSubscription$(
        window,
        "popstate",
        handler,
        false,
        null);
==========

to

==========
Store-forwarding for ReadTypeVariable

Example 1:

    var v0 = H.setRuntimeTypeInfo(new P.ListQueue(null, 0, 0, 0), [$E]);
    v0._table = J.JSArray_JSArray$list(8, H.getTypeArgumentByIndex(v0, 0));
-->
    var v0 = H.setRuntimeTypeInfo(new P.ListQueue(null, 0, 0, 0), [$E]);
    v0._table = J.JSArray_JSArray$list(8, $E);

Example 2 (enabling scalar replacement):

    this._historyTracking = W._EventStreamSubscription$(
        (v0 = H.setRuntimeTypeInfo(
              new W._EventStream(window, "popstate", false), [null]))._target,
        v0._eventType,
        handler,
        v0._useCapture,
        H.getTypeArgumentByIndex(v0, 0));
-->
    this._historyTracking = W._EventStreamSubscription$(
        window,
        "popstate",
        handler,
        false,
        null);

R=asgerf@google.com

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

[sra1, 2016-01-27 23:46:52]

Committed patchset #2 (id:60001) manually as
e1386ce2100ab3094f579b3c83f88cb01b11ef4e (presubmit successful).