Optimize is-check to instanceof when it eliminates an interceptor

Optimize is-check to instanceof when it eliminates an interceptor

See http://dartbug.com/22016 for discussion

R=floitsch@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43058

[sra1, 2015-01-16 05:15:54]

Patchset #1 (id:1) has been deleted

[sra1, 2015-01-16 05:16:02]

Patchset #1 (id:20001) has been deleted

[sra1, 2015-01-16 05:29:24]

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

[sra1, 2015-01-16 05:29:24]

What do you think?

There is a scattering of type tests that benefit in most programs or tests -
often in methods like Duration.== which tests "other is Duration".

There is a test like this in a hot Angular2 loop.  I'll see if I can get
benchmark results for that.

[floitsch, 2015-01-16 14:14:34]

LGTM.

https://codereview.chromium.org/829913006/diff/40001/pkg/compiler/lib/src/ssa...
File pkg/compiler/lib/src/ssa/codegen.dart (right):

https://codereview.chromium.org/829913006/diff/40001/pkg/compiler/lib/src/ssa...
pkg/compiler/lib/src/ssa/codegen.dart:2314:
backend.emitter.constructorAccess(type.element);
Please confirm that `constructorAccess` is correct. (I believe it is, but a
second opinion is helpful).
In the new emitter the constructor-access might return the stub (at least until
there really was a construction).
Normally the test should simply fail in these cases (which makes sense, since no
instantiated object exists yet).

Later the rhs might change (probably leading to a deopt), but at that point we
are obviously safe.

https://codereview.chromium.org/829913006/diff/40001/pkg/compiler/lib/src/ssa...
File pkg/compiler/lib/src/ssa/interceptor_simplifier.dart (right):

https://codereview.chromium.org/829913006/diff/40001/pkg/compiler/lib/src/ssa...
pkg/compiler/lib/src/ssa/interceptor_simplifier.dart:23: * 5) Some HIs
operations on a interceptor are replaced with a HIs version that
on an interceptor

https://codereview.chromium.org/829913006/diff/40001/pkg/compiler/lib/src/ssa...
pkg/compiler/lib/src/ssa/interceptor_simplifier.dart:62: // available in a local
variable.
If it's already in a local, let's just use it.
But maybe there are good examples where a change would make sense...

https://codereview.chromium.org/829913006/diff/40001/pkg/compiler/lib/src/ssa...
File pkg/compiler/lib/src/ssa/nodes.dart (right):

https://codereview.chromium.org/829913006/diff/40001/pkg/compiler/lib/src/ssa...
pkg/compiler/lib/src/ssa/nodes.dart:2457: TypeMask type, [bool
this.useInstanceOf = false])
I would make useInstanceOf a named argument.

[sra1, 2015-01-20 20:07:28]

PTAL

https://chromiumcodereview.appspot.com/829913006/diff/40001/pkg/compiler/lib/...
File pkg/compiler/lib/src/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/829913006/diff/40001/pkg/compiler/lib/...
pkg/compiler/lib/src/ssa/codegen.dart:2314:
backend.emitter.constructorAccess(type.element);
On 2015/01/16 14:14:33, floitsch wrote:
> Please confirm that `constructorAccess` is correct. (I believe it is, but a
> second opinion is helpful).

Verified.

> In the new emitter the constructor-access might return the stub (at least
until
> there really was a construction).
> Normally the test should simply fail in these cases (which makes sense, since
no
> instantiated object exists yet).
> 
> Later the rhs might change (probably leading to a deopt), but at that point we
> are obviously safe.

Good points.
Many instances are in methods of the class, e.g.  Duration.== has the check
"other is Duration".  These cases can only be reached after construction.

I think the risks are low since we are using 'instanceof' only if we can
eliminate a getInterceptor call.

I wonder if instanceof might be better than a property test for
self-intercepting classes where we 'know' that the property test will be
megamorphic?

https://chromiumcodereview.appspot.com/829913006/diff/40001/pkg/compiler/lib/...
File pkg/compiler/lib/src/ssa/interceptor_simplifier.dart (right):

https://chromiumcodereview.appspot.com/829913006/diff/40001/pkg/compiler/lib/...
pkg/compiler/lib/src/ssa/interceptor_simplifier.dart:23: * 5) Some HIs
operations on a interceptor are replaced with a HIs version that
On 2015/01/16 14:14:34, floitsch wrote:
> on an interceptor

Done.

https://chromiumcodereview.appspot.com/829913006/diff/40001/pkg/compiler/lib/...
pkg/compiler/lib/src/ssa/interceptor_simplifier.dart:62: // available in a local
variable.
On 2015/01/16 14:14:34, floitsch wrote:
> If it's already in a local, let's just use it.
> But maybe there are good examples where a change would make sense...

I was able to contrive an example where this removed the last reference to an
interceptor so I will leave it in for now.

https://chromiumcodereview.appspot.com/829913006/diff/40001/pkg/compiler/lib/...
File pkg/compiler/lib/src/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/829913006/diff/40001/pkg/compiler/lib/...
pkg/compiler/lib/src/ssa/nodes.dart:2457: TypeMask type, [bool
this.useInstanceOf = false])
On 2015/01/16 14:14:34, floitsch wrote:
> I would make useInstanceOf a named argument.

Done.

[floitsch, 2015-01-20 20:15:42]

Still LGTM.

https://chromiumcodereview.appspot.com/829913006/diff/60001/pkg/compiler/lib/...
File pkg/compiler/lib/src/ssa/interceptor_simplifier.dart (right):

https://chromiumcodereview.appspot.com/829913006/diff/60001/pkg/compiler/lib/...
pkg/compiler/lib/src/ssa/interceptor_simplifier.dart:59: //return false;
commented code.

[sra1, 2015-01-22 02:24:07]

Committed patchset #3 (id:80001) manually as 43058 (presubmit successful).