Adding check or trust type checks to builder_kernel.dart.

Adding check or trust type checks to builder_kernel.dart.

BUG= https://github.com/dart-lang/sdk/issues/27394
R=het@google.com, sigmund@google.com, sra@google.com

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

[Emily Fortuna, 2016-11-11 23:03:08]

Patchset #2 (id:20001) has been deleted

[Emily Fortuna, 2016-11-11 23:03:22]

Patchset #1 (id:1) has been deleted

[Emily Fortuna, 2016-11-11 23:21:33]

Patchset #1 (id:40001) has been deleted

[Emily Fortuna, 2016-11-11 23:22:36]

Description was changed from

==========
Adding check or trust type checks to builder_kernel.dart.

BUG=
==========

to

==========
Adding check or trust type checks to builder_kernel.dart.

BUG= https://github.com/dart-lang/sdk/issues/27394
==========

[Emily Fortuna, 2016-11-11 23:22:37]

efortuna@google.com changed reviewers:
+ het@google.com, sra@google.com

[Emily Fortuna, 2016-11-11 23:23:11]

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:206: Element get sourceElement =>
astAdapter.getElement(target);
really not sure about this as being what sourceElement was intended to be. lmk
-- all the tests pass..... :-P

[sra1, 2016-11-12 00:31:19]

initial comments

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:206: Element get sourceElement =>
astAdapter.getElement(target);
On 2016/11/11 23:23:11, Emily Fortuna wrote:
> really not sure about this as being what sourceElement was intended to be. lmk
> -- all the tests pass..... :-P

SourceElement is used to mark HInstructions with the source variable that they
correspond to.  This helps us make up names that are the same as the original
code, and I think it is used in checks (like type assertions) to generate

    function(s) {
      H.checkString(s);
      return s.length;
    }

instead of this uglier code that is closer to the SSA graph:

    function(s) {
      var t1;
      t1 = H.checkString(s);
      return t1.length;
    }

i.e. to detect which different HInstructions can use the same JS variable.


So look at the output of --checked code to verify if it looks like the first
example.
We might generate

     return H.checkString(s).length;

if do something like  return s.length + s.codeUnitAt(0);

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/graph_builder.dart (right):

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/graph_builder.dart:222: class TypeBuilder implements
DartTypeVisitor<dynamic, GraphBuilder> {
There is a lot of duplication here. Is it necessary (is DartType here different
to DartType in the ssa/builder.dart version?

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/type_verifier.dart (right):

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/type_verifier.dart:16: /// Functions to insert type
checking, coercion, and verification depending on
What is 'verification'?

I'm wondering if these methods should just be part of GraphBuilder?

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/type_verifier.dart:135: builder.push(new
HTypeInfoReadVariable(variable, target, builder.backend.dynamicType)
dartfmt -w the files

[Emily Fortuna, 2016-11-14 17:47:56]

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:206: Element get sourceElement =>
astAdapter.getElement(target);
On 2016/11/12 00:31:19, sra1 wrote:
> On 2016/11/11 23:23:11, Emily Fortuna wrote:
> > really not sure about this as being what sourceElement was intended to be.
lmk
> > -- all the tests pass..... :-P
> 
> SourceElement is used to mark HInstructions with the source variable that they
> correspond to.  This helps us make up names that are the same as the original
> code, and I think it is used in checks (like type assertions) to generate
> 
>     function(s) {
>       H.checkString(s);
>       return s.length;
>     }
> 
> instead of this uglier code that is closer to the SSA graph:
> 
>     function(s) {
>       var t1;
>       t1 = H.checkString(s);
>       return t1.length;
>     }
> 
> i.e. to detect which different HInstructions can use the same JS variable.
> 
> 
> So look at the output of --checked code to verify if it looks like the first
> example.
> We might generate
> 
>      return H.checkString(s).length;
> 
> if do something like  return s.length + s.codeUnitAt(0);

Hmmm what dart code should produce that compiles to that? 

import 'package:args/args.dart';

int foo(String s) {
  return s?.length;
}
main(List<String> arguments) {
  final parser = new ArgParser();
  var argResults = parser.parse(arguments);
  print(foo(argResults.rest));
}

but I wasn't seeing the H.checkString call?

The real question is what is the equivalent in the builder_kernel.dart
environment?

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/graph_builder.dart (right):

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/graph_builder.dart:222: class TypeBuilder implements
DartTypeVisitor<dynamic, GraphBuilder> {
On 2016/11/12 00:31:19, sra1 wrote:
> There is a lot of duplication here. Is it necessary (is DartType here
different
> to DartType in the ssa/builder.dart version?

sorry I hadn't cut the code from builder.dart. the code is indeed identical but
I intended to move it here.

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/type_verifier.dart (right):

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/type_verifier.dart:16: /// Functions to insert type
checking, coercion, and verification depending on
On 2016/11/12 00:31:19, sra1 wrote:
> What is 'verification'?
> 
> I'm wondering if these methods should just be part of GraphBuilder?

talked offline. We renamed this class (and another class)

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/type_verifier.dart:135: builder.push(new
HTypeInfoReadVariable(variable, target, builder.backend.dynamicType)
On 2016/11/12 00:31:19, sra1 wrote:
> dartfmt -w the files

Done.

[sra1, 2016-11-14 18:00:21]

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:206: Element get sourceElement =>
astAdapter.getElement(target);
On 2016/11/14 17:47:56, Emily Fortuna wrote:
> On 2016/11/12 00:31:19, sra1 wrote:
> > On 2016/11/11 23:23:11, Emily Fortuna wrote:
> > > really not sure about this as being what sourceElement was intended to be.
> lmk
> > > -- all the tests pass..... :-P
> > 
> > SourceElement is used to mark HInstructions with the source variable that
they
> > correspond to.  This helps us make up names that are the same as the
original
> > code, and I think it is used in checks (like type assertions) to generate
> > 
> >     function(s) {
> >       H.checkString(s);
> >       return s.length;
> >     }
> > 
> > instead of this uglier code that is closer to the SSA graph:
> > 
> >     function(s) {
> >       var t1;
> >       t1 = H.checkString(s);
> >       return t1.length;
> >     }
> > 
> > i.e. to detect which different HInstructions can use the same JS variable.
> > 
> > 
> > So look at the output of --checked code to verify if it looks like the first
> > example.
> > We might generate
> > 
> >      return H.checkString(s).length;
> > 
> > if do something like  return s.length + s.codeUnitAt(0);
> 
> Hmmm what dart code should produce that compiles to that? 
> 
> import 'package:args/args.dart';
> 
> int foo(String s) {
>   return s?.length;
> }
> main(List<String> arguments) {
>   final parser = new ArgParser();
>   var argResults = parser.parse(arguments);
>   print(foo(argResults.rest));
> }
> 
> but I wasn't seeing the H.checkString call?
> 
> The real question is what is the equivalent in the builder_kernel.dart
> environment?

"return s.length + s.codeUnitAt(0);" is the suggested Dart code to ensure the
checked result is used twice.

You can call the function twice from main,  foo("Hi") and foo(null).
You can make the function be not inlined by having a second return, or by using
@NoInline(), imported from pkg:expect/expect.dart

[sra1, 2016-11-14 18:00:30]

lgtm

[Siggi Cherem (dart-lang), 2016-11-14 18:30:20]

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

[Siggi Cherem (dart-lang), 2016-11-14 18:30:21]

lgtm, very nice!

https://codereview.chromium.org/2479323003/diff/80001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2479323003/diff/80001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:205: Element get sourceElement =>
astAdapter.getElement(target);
let's add a small TODO here that we need to update this when we implement
inilining.

https://codereview.chromium.org/2479323003/diff/80001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/graph_builder.dart (right):

https://codereview.chromium.org/2479323003/diff/80001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/graph_builder.dart:43: CodegenRegistry get registry;
remove field & import above? (it appears it's no longer needed?)

[Harry Terkelsen, 2016-11-14 18:37:06]

lgtm

https://codereview.chromium.org/2479323003/diff/80001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/type_builder.dart (right):

https://codereview.chromium.org/2479323003/diff/80001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/type_builder.dart:56: HInstruction
potentiallyCheckOrTrustType(HInstruction original, DartType type,
there are a few places in the code where you do "if
(typeBuilder.checkOrTrustTypes) { typeBuilder.potentiallyCheckOrTrustType(...)
}" but potentiallyCheckOrTrust type doesn't do anything if checkOrTrustTypes is
false.

[Emily Fortuna, 2016-11-14 19:03:39]

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2479323003/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:206: Element get sourceElement =>
astAdapter.getElement(target);
On 2016/11/14 18:00:20, sra1 wrote:
> On 2016/11/14 17:47:56, Emily Fortuna wrote:
> > On 2016/11/12 00:31:19, sra1 wrote:
> > > On 2016/11/11 23:23:11, Emily Fortuna wrote:
> > > > really not sure about this as being what sourceElement was intended to
be.
> > lmk
> > > > -- all the tests pass..... :-P
> > > 
> > > SourceElement is used to mark HInstructions with the source variable that
> they
> > > correspond to.  This helps us make up names that are the same as the
> original
> > > code, and I think it is used in checks (like type assertions) to generate
> > > 
> > >     function(s) {
> > >       H.checkString(s);
> > >       return s.length;
> > >     }
> > > 
> > > instead of this uglier code that is closer to the SSA graph:
> > > 
> > >     function(s) {
> > >       var t1;
> > >       t1 = H.checkString(s);
> > >       return t1.length;
> > >     }
> > > 
> > > i.e. to detect which different HInstructions can use the same JS variable.
> > > 
> > > 
> > > So look at the output of --checked code to verify if it looks like the
first
> > > example.
> > > We might generate
> > > 
> > >      return H.checkString(s).length;
> > > 
> > > if do something like  return s.length + s.codeUnitAt(0);
> > 
> > Hmmm what dart code should produce that compiles to that? 
> > 
> > import 'package:args/args.dart';
> > 
> > int foo(String s) {
> >   return s?.length;
> > }
> > main(List<String> arguments) {
> >   final parser = new ArgParser();
> >   var argResults = parser.parse(arguments);
> >   print(foo(argResults.rest));
> > }
> > 
> > but I wasn't seeing the H.checkString call?
> > 
> > The real question is what is the equivalent in the builder_kernel.dart
> > environment?
> 
> "return s.length + s.codeUnitAt(0);" is the suggested Dart code to ensure the
> checked result is used twice.
> 
> You can call the function twice from main,  foo("Hi") and foo(null).
> You can make the function be not inlined by having a second return, or by
using
> @NoInline(), imported from pkg:expect/expect.dart

Okay. Code looks like option one, so I think we're good:

H.stringTypeCheck(s);
return s.length + C.JSString_methods.codeUnitAt$1(s, 0);

https://codereview.chromium.org/2479323003/diff/80001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2479323003/diff/80001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:205: Element get sourceElement =>
astAdapter.getElement(target);
On 2016/11/14 18:30:21, Siggi Cherem (dart-lang) wrote:
> let's add a small TODO here that we need to update this when we implement
> inilining.

Done.

https://codereview.chromium.org/2479323003/diff/80001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/graph_builder.dart (right):

https://codereview.chromium.org/2479323003/diff/80001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/graph_builder.dart:43: CodegenRegistry get registry;
On 2016/11/14 18:30:21, Siggi Cherem (dart-lang) wrote:
> remove field & import above? (it appears it's no longer needed?)

It's used in type_builder.dart

https://codereview.chromium.org/2479323003/diff/80001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/type_builder.dart (right):

https://codereview.chromium.org/2479323003/diff/80001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/type_builder.dart:56: HInstruction
potentiallyCheckOrTrustType(HInstruction original, DartType type,
On 2016/11/14 18:37:06, Harry Terkelsen wrote:
> there are a few places in the code where you do "if
> (typeBuilder.checkOrTrustTypes) { typeBuilder.potentiallyCheckOrTrustType(...)
> }" but potentiallyCheckOrTrust type doesn't do anything if checkOrTrustTypes
is
> false.

I saw that too, when I was porting the original code from builder.dart to
builder_kernel.dart. The trick is if you look at the control flow, if you take
out that check , say in builder.dart 1571, then you'd just be returning the
result of potentiallyCheckOrTrustType which is not what we want if
checkOrTrustTypes is false. The other way to write this would be to call
potentiallyCheckOrTrustType and duplicate the check if the returned result is
the same as before calling, but that doesn't seem any cleaner than just calling
checkOrTrustTypes....

[Emily Fortuna, 2016-11-14 19:08:48]

Description was changed from

==========
Adding check or trust type checks to builder_kernel.dart.

BUG= https://github.com/dart-lang/sdk/issues/27394
==========

to

==========
Adding check or trust type checks to builder_kernel.dart.

BUG= https://github.com/dart-lang/sdk/issues/27394
R=het@google.com, sigmund@google.com, sra@google.com

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

[Emily Fortuna, 2016-11-14 19:08:49]

Committed patchset #3 (id:100001) manually as
c0554489bd431d6c8a5723495d09c27a5860ddd1 (presubmit successful).