dart2js: Trust type annotations more often with --trust-type-annotation.

dart2js: Trust type annotations more often with --trust-type-annotation.

R=sra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=41636
Reverted: https://code.google.com/p/dart/source/detail?r=41637

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

[floitsch, 2014-11-05 19:59:22]

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

[floitsch, 2014-11-05 19:59:23]

I'm still fighting with the test (mainly the mock-compiler), but I have tested
it by hand.

[sra1, 2014-11-06 03:11:29]

LGTM

This looks like it would fix the errors I get with developer mode dart2js with
--trust-types.

[floitsch, 2014-11-06 16:57:52]

PTAL.
Turns out that we didn't use the checked/trusted information as much as we could
(see the type-propagator).

https://codereview.chromium.org/707493003/diff/20001/tests/compiler/dart2js/t...
File tests/compiler/dart2js/trust_type_annotations2_test.dart (right):

https://codereview.chromium.org/707493003/diff/20001/tests/compiler/dart2js/t...
tests/compiler/dart2js/trust_type_annotations2_test.dart:26: var compiler =
compilerFor(TEST, uri, disableInlining: false/*, trustTypeAnnotations: true*/);
I know, long line.
I will update the test. At the moment it doesn't pass yet.

[sra1, 2014-11-06 17:47:51]

https://codereview.chromium.org/707493003/diff/20001/pkg/compiler/lib/src/ssa...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/707493003/diff/20001/pkg/compiler/lib/src/ssa...
pkg/compiler/lib/src/ssa/builder.dart:4148: assert(stack.last is HInvokeStatic
|| stack.last == newObject);
I get an assert here when compiling swarm with dart2js_developer

The compiler crashed:
'file:///usr/local/google/home/sra/dart-all/dart/pkg/compiler/lib/src/ssa/builder.dart':
Failed assertion: line 4148 pos 12: 'stack.last is HInvokeStatic || stack.last
== newObject' is not true.
#0      SsaBuilder.callSetRuntimeTypeInfo
(file:///usr/local/google/home/sra/dart-all/dart/pkg/compiler/lib/src/ssa/builder.dart:4148:12)
#1      SsaBuilder.setRtiIfNeeded
(file:///usr/local/google/home/sra/dart-all/dart/pkg/compiler/lib/src/ssa/builder.dart:5115:34)
#2      SsaBuilder.visitLiteralList
(file:///usr/local/google/home/sra/dart-all/dart/pkg/compiler/lib/src/ssa/builder.dart:5133:35)
#3      LiteralList.accept
(file:///usr/local/google/home/sra/dart-all/dart/pkg/compiler/lib/src/tree/nodes.dart:980:54)


The code being compiled is ordinary code in js_helper:

  static String stringFromCodePoints(codePoints) {
    List<int> a = <int>[];
    for (var i in codePoints) {

I suspect stack.last is HTypeKnown

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

https://codereview.chromium.org/707493003/diff/20001/pkg/compiler/lib/src/ssa...
pkg/compiler/lib/src/ssa/codegen.dart:2576: assert(compiler.trustTypeAnnotations
||
Does inserting the HTypeKnown during inlining not catch this?
When node.checkInput has dynamic type this leads to the fixnum test for int, (x
| 0) === x, rather than the correct
typeof x == "number" && x == Math.floor(x)

from Sort__dualPivotQuicksort() in swarm
----------
        t2 = J.getInterceptor(comp);
        if (t2.$eq(comp, 0))
          continue;
        if (t2.$lt(comp, 0)) {
---------->
        if (comp === 0)
          continue;
        if (comp !== (comp | 0))    //  
          return comp.$lt();
        if (comp < 0) {
----------

A test for this would be to sort with a compare function:
 
myCompare(a,b) => 1000000000000 * a.compareTo(b).


To be clear: this is an existing bug with the old version of
--trust-type-annotations.  It is what I noticed in the customer code last week.

[floitsch, 2014-11-07 15:50:13]

PTAL.
I "fixed" the crash in Swarm by not adding an HTypeKnown for dynamic or Object.
Tbh I don't know why this is necessary, but I'm guessing that we were hitting a
special case where creating the type was interfering with trust-type-annotation.

I'm starting to wonder, if we should just add checked-nodes, and then replace
them all in a separate pass to HTypeKnown.

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

https://codereview.chromium.org/707493003/diff/20001/pkg/compiler/lib/src/ssa...
pkg/compiler/lib/src/ssa/codegen.dart:2576: assert(compiler.trustTypeAnnotations
||
On 2014/11/06 17:47:51, sra1 wrote:
> Does inserting the HTypeKnown during inlining not catch this?
> When node.checkInput has dynamic type this leads to the fixnum test for int,
(x
> | 0) === x, rather than the correct
> typeof x == "number" && x == Math.floor(x)
> 
> from Sort__dualPivotQuicksort() in swarm
> ----------
>         t2 = J.getInterceptor(comp);
>         if (t2.$eq(comp, 0))
>           continue;
>         if (t2.$lt(comp, 0)) {
> ---------->
>         if (comp === 0)
>           continue;
>         if (comp !== (comp | 0))    //  
>           return comp.$lt();
>         if (comp < 0) {
> ----------
> 
> A test for this would be to sort with a compare function:
>  
> myCompare(a,b) => 1000000000000 * a.compareTo(b).
> 
> 
> To be clear: this is an existing bug with the old version of
> --trust-type-annotations.  It is what I noticed in the customer code last
week.

The problem seems to be that HTypeKnown nodes are removed before we enter
codegen. However, here we try to read the type of the input which, without the
HTypeKnown, isn't "correct" anymore.

[floitsch, 2014-11-07 16:29:32]

On 2014/11/07 15:50:13, floitsch wrote:
> PTAL.
> I "fixed" the crash in Swarm by not adding an HTypeKnown for dynamic or
Object.
> Tbh I don't know why this is necessary, but I'm guessing that we were hitting
a
> special case where creating the type was interfering with
trust-type-annotation.
> 
> I'm starting to wonder, if we should just add checked-nodes, and then replace
> them all in a separate pass to HTypeKnown.
> 
>
https://codereview.chromium.org/707493003/diff/20001/pkg/compiler/lib/src/ssa...
> File pkg/compiler/lib/src/ssa/codegen.dart (right):
> 
>
https://codereview.chromium.org/707493003/diff/20001/pkg/compiler/lib/src/ssa...
> pkg/compiler/lib/src/ssa/codegen.dart:2576:
assert(compiler.trustTypeAnnotations
> ||
> On 2014/11/06 17:47:51, sra1 wrote:
> > Does inserting the HTypeKnown during inlining not catch this?
> > When node.checkInput has dynamic type this leads to the fixnum test for int,
> (x
> > | 0) === x, rather than the correct
> > typeof x == "number" && x == Math.floor(x)
> > 
> > from Sort__dualPivotQuicksort() in swarm
> > ----------
> >         t2 = J.getInterceptor(comp);
> >         if (t2.$eq(comp, 0))
> >           continue;
> >         if (t2.$lt(comp, 0)) {
> > ---------->
> >         if (comp === 0)
> >           continue;
> >         if (comp !== (comp | 0))    //  
> >           return comp.$lt();
> >         if (comp < 0) {
> > ----------
> > 
> > A test for this would be to sort with a compare function:
> >  
> > myCompare(a,b) => 1000000000000 * a.compareTo(b).
> > 
> > 
> > To be clear: this is an existing bug with the old version of
> > --trust-type-annotations.  It is what I noticed in the customer code last
> week.
> 
> The problem seems to be that HTypeKnown nodes are removed before we enter
> codegen. However, here we try to read the type of the input which, without the
> HTypeKnown, isn't "correct" anymore.

I ran through all language test with --trust-type-annotations, and only the
following complained:
  language/arithmetic_test: RuntimeError
  language/bool_test: RuntimeError
  language/factory_return_type_checked_test: RuntimeError
  language/field_override_test/02: RuntimeError
  language/substring_test/01: RuntimeError

They all test dynamic tests that are invalidated by --trust-type-annotations. So
these are fine.

[sra1, 2014-11-07 16:42:49]

lgtm

[floitsch, 2014-11-10 15:50:39]

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

[floitsch, 2014-11-10 16:53:14]

Fixed the spelling of the flag to dart2js.
Resubmitting.

[floitsch, 2014-11-10 16:54:09]

Committed patchset #5 (id:80001) manually as 41639 (presubmit successful).