dart2js cps: 'is' checks on types with type arguments.

dart2js cps: 'is' checks on types with type arguments.

R=karlklose@google.com

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

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

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

[asgerf, 2015-05-26 17:52:10]

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

[karlklose, 2015-05-27 07:38:35]

LGTM with comments:

https://codereview.chromium.org/1161683002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1161683002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2310: type, [], k, isTypeTest:
isTypeTest));
Please add type arguments to list literals (also in below). This could also be
const.

https://codereview.chromium.org/1161683002/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/1161683002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:362: assert(!type.isObject
&& !type.isDynamic);
Also assert(type is! InterfaceType || (type as InterfaceType).element !=
nullClass)?

https://codereview.chromium.org/1161683002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:363: if (type is
InterfaceType) {
I think we should keep the case for no type arguments separate and always
provide all arguments to the helper.

Using less arguments than declared is not free in JavaScript engines and we can
add a different helper if the explicit null literals turn out to be a problem
with code size.  Also, we do not need to emit the helper, if there are no
generic type checks in the program.

[asgerf, 2015-05-27 09:19:16]

https://codereview.chromium.org/1161683002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1161683002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2310: type, [], k, isTypeTest:
isTypeTest));
On 2015/05/27 07:38:34, karlklose wrote:
> Please add type arguments to list literals (also in below). This could also be
> const.

Done.

https://codereview.chromium.org/1161683002/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/1161683002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:362: assert(!type.isObject
&& !type.isDynamic);
On 2015/05/27 07:38:34, karlklose wrote:
> Also assert(type is! InterfaceType || (type as InterfaceType).element !=
> nullClass)?

The assertions are mostly there to add clarity. I left this one out because I
felt such a verbose assertion would have the opposite effect.

We would also get a method in the glue class that we don't actually need
(getting the null class), which kind of defeats the purpose of having the glue
class.

https://codereview.chromium.org/1161683002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:363: if (type is
InterfaceType) {
On 2015/05/27 07:38:35, karlklose wrote:
> I think we should keep the case for no type arguments separate and always
> provide all arguments to the helper.
> 
> Using less arguments than declared is not free in JavaScript engines and we
can
> add a different helper if the explicit null literals turn out to be a problem
> with code size.  Also, we do not need to emit the helper, if there are no
> generic type checks in the program.

I should have clarified in the CL description.

The case for a non-generic type will be detected in a CPS optimization phase and
replaced by a specialized IR node. The reason being that the value should
usually be intercepted (and hence shared if possible) in the non-generic case,
but *only* in the non-generic case. In the generic case, the interceptor is not
needed.

And we don't want the TypeOperator to have some silly condition like "the value
must be an interceptor unless there are type arguments".

Good point about the missing arguments. I'll insert nulls.

[asgerf, 2015-05-27 11:45:43]

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

[asgerf, 2015-05-27 12:14:11]

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