dart2js cps: Track underlying value of interceptors in type propagation.

dart2js cps: Track underlying value of interceptors in type propagation.

BUG=
R=karlklose@google.com

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

[asgerf, 2015-05-26 10:39:13]

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

[karlklose, 2015-05-26 11:05:23]

LGTM, but do you have a change or test in preparation which actually uses the
value?

[asgerf, 2015-05-26 11:08:11]

On 2015/05/26 11:05:23, karlklose wrote:
> LGTM, but do you have a change or test in preparation which actually uses the
> value?

There were failing tests already. The status file changes have been uploaded
now.

[sra1, 2015-05-26 21:55:44]

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

[sra1, 2015-05-26 21:55:45]

DBC.  I think I might be being confused by the terminology of the comments.

I don't think that in
  y = getInterceptor(x)
that y can have the 'same' type as x.

One thing I have wanted to do in the SSA backend is recognize calls to
getInterceptor and replace with an Interceptor node to get better CSE.
This would remove any partitioning of values on whether or not they are a Dart
value vs an interceptor.

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

https://codereview.chromium.org/1154173002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:882: // We currently do not
track whether a value is intercepted or not.
What do you mean?
The interceptor (method table) is a pure function of the value.
The value is no different whether or not it has a method table computed for it.

https://codereview.chromium.org/1154173002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:883: // An intercepted value
is never a constant, but a non-constant can either
In SSA we replace  getInterceptor(x) with a constant value for the interceptor. 
That leads to JS code that looks like:

   C.JSArray_methods.forEach(x, new closureY());
   s = C.JSString_methods.trim$0(" hello ");

This replacement happens very late, since it is a 'global' optimization. If the
potential constant interceptor is dominated by necessary call to getInterceptor
there is no advantage to replacing by a constant.

https://codereview.chromium.org/1154173002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:888: setValue(node,
nonConstant(value.type));
It is not correct for the result of getInterceptor(x) to be modeled by
the type of x.  In SSA we don't bother with a type since all uses are handled
specially.  We use 'dynamic'.

[asgerf, 2015-05-27 08:45:23]

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

https://codereview.chromium.org/1154173002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:882: // We currently do not
track whether a value is intercepted or not.
On 2015/05/26 21:55:45, sra1 wrote:
> What do you mean?
> The interceptor (method table) is a pure function of the value.
> The value is no different whether or not it has a method table computed for
it.

I mean when the abstract value of something is nonConst(T), the concrete value
can either be an instance of T, or the result of getInterceptor that was called
with an argument of type T.

I used the term "intercepted value" to mean the result of getInterceptor, not
its argument.

https://codereview.chromium.org/1154173002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:883: // An intercepted value
is never a constant, but a non-constant can either
On 2015/05/26 21:55:44, sra1 wrote:
> In SSA we replace  getInterceptor(x) with a constant value for the
interceptor. 
> That leads to JS code that looks like:
> 
>    C.JSArray_methods.forEach(x, new closureY());
>    s = C.JSString_methods.trim$0(" hello ");
> 
> This replacement happens very late, since it is a 'global' optimization. If
the
> potential constant interceptor is dominated by necessary call to
getInterceptor
> there is no advantage to replacing by a constant.

Here I was referring to the abstract value of kind 'constant'.

https://codereview.chromium.org/1154173002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:888: setValue(node,
nonConstant(value.type));
On 2015/05/26 21:55:44, sra1 wrote:
> It is not correct for the result of getInterceptor(x) to be modeled by
> the type of x.  In SSA we don't bother with a type since all uses are handled
> specially.  We use 'dynamic'.

Correctness here is a matter of definition. I changed the abstraction, so
pedantically it was by definition then the correct thing to do. The crucial
point is that none of the other transfer functions or optimizations became
unsound under the new definition, although not for obvious reasons.

But it's clearly confusing, so I'll just set it to dynamic. Type propagation is
in for an overhaul anyway.

The concrete problem I was fixing showed up in visitInvokeMethod where it
(correctly) bails out if the receiver type is 'nothing'. The important part is
just that interceptors are considered to be actual values and not 'nothing'.

[asgerf, 2015-06-01 09:19:41]

Committed patchset #6 (id:100001) manually as
bd4e1d726f1d9fedd0cd46802c048e88b58d8cd0 (presubmit successful).