dart2js cps: Generate interceptors at use-site and share afterwards.

dart2js cps: Generate interceptors at use-site and share afterwards.

Previously, interceptors were generated at the definition-site to ensure
sharing, and sometimes sunk to its use-site if desirable.

Now they are (again) generated at use-site, then pulled out of loops,
and replaced by dominating calls to getInterceptor.

This means interceptors are only computed if they really are needed,
but on the other hand, they are sometimes computed more than once, when
there are uses that don't have a common dominator.

Interceptor constants are currently not shared/hoisted as it is not
clear when this is beneficial.

R=kmillikin@google.com

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

[asgerf, 2015-08-25 14:00:42]

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

[asgerf, 2015-08-25 14:00:43]

https://codereview.chromium.org/1313783003/diff/1/tests/compiler/dart2js_extr...
File tests/compiler/dart2js_extra/dart2js_extra.status (right):

https://codereview.chromium.org/1313783003/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/dart2js_extra.status:73: 23432_test: RuntimeError #
Issue 23432
It sort of passed for the wrong reason, and also seems to pass for the wrong
reason in the SSA backend.

[Kevin Millikin (Google), 2015-08-28 11:27:28]

LGTM.

[asgerf, 2015-08-28 14:02:21]

Committed patchset #4 (id:60001) manually as
ade4822108cc1e61dea7120da46e2bb8518715c4 (presubmit successful).

[sra1, 2015-08-31 18:12:07]

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

[sra1, 2015-08-31 18:12:08]

https://codereview.chromium.org/1313783003/diff/1/tests/compiler/dart2js_extr...
File tests/compiler/dart2js_extra/dart2js_extra.status (right):

https://codereview.chromium.org/1313783003/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/dart2js_extra.status:73: 23432_test: RuntimeError #
Issue 23432
On 2015/08/25 14:00:42, asgerf wrote:
> It sort of passed for the wrong reason, and also seems to pass for the wrong
> reason in the SSA backend.

What is the wrong reason?

[asgerf, 2015-08-31 18:23:05]

https://codereview.chromium.org/1313783003/diff/1/tests/compiler/dart2js_extr...
File tests/compiler/dart2js_extra/dart2js_extra.status (right):

https://codereview.chromium.org/1313783003/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/dart2js_extra.status:73: 23432_test: RuntimeError #
Issue 23432
On 2015/08/31 18:12:07, sra1 wrote:
> On 2015/08/25 14:00:42, asgerf wrote:
> > It sort of passed for the wrong reason, and also seems to pass for the wrong
> > reason in the SSA backend.
> 
> What is the wrong reason?

Ah, right. I was trying to get to the bottom of things, but then got
side-tracked.

There is a call to getInterceptor that gets a numeric argument, but JSNumber is
not an intercepted class because numbers don't respond to that selector. The
call intentionally fails at runtime.

In CPS, the getInterceptor call returns the number itself, and the resulting
exception message is not rewritten into a nice exception message. It says
something like "getInterceptor(x).blah" is not a method where it was supposed to
say something like "12345 does not have a method blah".

In SSA, the getInterceptor call has a case for UnknownJavaScriptObject which is
only there because the program contains a try/catch. The number is (on purpose?)
classified as such an unknown object. The unknown object's method table then
responds to the message by throwing a nice-looking exception message and passes
the test.

The UnknownJavaScriptObject case seems to go away if there is no try/catch.

I can make SSA generate just as bad error messages by removing all try/catches
from the program, but we can't make a test case for it because it needs a
try/catch to inspect the error message.

I don't know the official strategy for getting nice-looking exception message.
Having the UnknownJavaScriptObject case isn't such a bad idea for handling
negative cases, but the condition under which that happens in the SSA is weird.

[sra1, 2015-08-31 18:45:18]

On 2015/08/31 18:23:05, asgerf wrote:
>
https://codereview.chromium.org/1313783003/diff/1/tests/compiler/dart2js_extr...
> File tests/compiler/dart2js_extra/dart2js_extra.status (right):
> 
>
https://codereview.chromium.org/1313783003/diff/1/tests/compiler/dart2js_extr...
> tests/compiler/dart2js_extra/dart2js_extra.status:73: 23432_test: RuntimeError
#
> Issue 23432
> On 2015/08/31 18:12:07, sra1 wrote:
> > On 2015/08/25 14:00:42, asgerf wrote:
> > > It sort of passed for the wrong reason, and also seems to pass for the
wrong
> > > reason in the SSA backend.
> > 
> > What is the wrong reason?
> 
> Ah, right. I was trying to get to the bottom of things, but then got
> side-tracked.
> 
> There is a call to getInterceptor that gets a numeric argument, but JSNumber
is
> not an intercepted class because numbers don't respond to that selector. The
> call intentionally fails at runtime.
> 
> In CPS, the getInterceptor call returns the number itself, and the resulting
> exception message is not rewritten into a nice exception message. It says
> something like "getInterceptor(x).blah" is not a method where it was supposed
to
> say something like "12345 does not have a method blah".
> 
> In SSA, the getInterceptor call has a case for UnknownJavaScriptObject which
is
> only there because the program contains a try/catch. The number is (on
purpose?)
> classified as such an unknown object. The unknown object's method table then
> responds to the message by throwing a nice-looking exception message and
passes
> the test.
> 
> The UnknownJavaScriptObject case seems to go away if there is no try/catch.
> 
> I can make SSA generate just as bad error messages by removing all try/catches
> from the program, but we can't make a test case for it because it needs a
> try/catch to inspect the error message.
> 
> I don't know the official strategy for getting nice-looking exception message.
> Having the UnknownJavaScriptObject case isn't such a bad idea for handling
> negative cases, but the condition under which that happens in the SSA is
weird.

Thanks for the details. I want to revisit the area of generating useful errors
and make it more consistent.
The current situation in SSA is, as you say, weird.