dart2js cps: 'is'-check directly against type variables.

dart2js cps: 'is'-check directly against type variables.

R=karlklose@google.com

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

[asgerf, 2015-05-27 13:38:19]

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

[karlklose, 2015-05-28 07:42:30]

LGTM.

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

https://codereview.chromium.org/1160763002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2673: // For type tests, we must
treat specially the rare cases where `null`
Move comment back into the if branch?

Also, the treatment of `null` is not the only special case that is handled here.
Maybe rewrite the comment to something like:
 // Handle statically known checks against [Object], [dynamic], which  
 // are trivial, and [Null], which can be decided by an identity    
 // comparison.

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

https://codereview.chromium.org/1160763002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:392: // We use the helper:
I am not sure these comments have much value. Maybe we should add documentation
to glue, where the helpers are located?

https://codereview.chromium.org/1160763002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:398: [value, typeValue]);
Add type arguments to list (also in line 386.)

https://codereview.chromium.org/1160763002/diff/1/tests/language/language_dar...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/1160763002/diff/1/tests/language/language_dar...
tests/language/language_dart2js.status:794: instanceof4_test/01 : RuntimeError
Are the RuntimeError failures bugs in the runtime type system of the CPS backend
or unrelated?

[asgerf, 2015-05-28 08:52:55]

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

https://codereview.chromium.org/1160763002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2673: // For type tests, we must
treat specially the rare cases where `null`
On 2015/05/28 07:42:30, karlklose wrote:
> Move comment back into the if branch?
> 
> Also, the treatment of `null` is not the only special case that is handled
here.
> Maybe rewrite the comment to something like:
>  // Handle statically known checks against [Object], [dynamic], which  
>  // are trivial, and [Null], which can be decided by an identity    
>  // comparison.

It should be clear that this is not an optimization. It MUST be done because
`null` satisfies these tests and the codegen for TypeOperator assumes that null
can never satisfy a type test.

I'll try to clarify and move it back into the branch.

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

https://codereview.chromium.org/1160763002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:392: // We use the helper:
On 2015/05/28 07:42:30, karlklose wrote:
> I am not sure these comments have much value. Maybe we should add
documentation
> to glue, where the helpers are located?

I added the doc comments to glue and removed this instance of the comment.

I decided to keep the one for checkSubtype since it gives meaning to some
variable names and explains why some things are nulled.

https://codereview.chromium.org/1160763002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:398: [value, typeValue]);
On 2015/05/28 07:42:30, karlklose wrote:
> Add type arguments to list (also in line 386.)

Done.

https://codereview.chromium.org/1160763002/diff/1/tests/language/language_dar...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/1160763002/diff/1/tests/language/language_dar...
tests/language/language_dart2js.status:794: instanceof4_test/01 : RuntimeError
On 2015/05/28 07:42:30, karlklose wrote:
> Are the RuntimeError failures bugs in the runtime type system of the CPS
backend
> or unrelated?

It's because we don't set runtime type information on list literals. I.e.
<int>[1,2,3] is just compiled to [1,2,3].

https://codereview.chromium.org/1160763002/diff/1/tests/language/language_dar...
tests/language/language_dart2js.status:1091: generic_field_mixin4_test :
RuntimeError
This is addressed by the other CL.

[asgerf, 2015-05-28 10:30:07]

Committed patchset #3 (id:40001) manually as
c29b7c646116020b7b5c177601a4bfd69aba14af (presubmit successful).