Working implementation of #29153 range check in as-casts.

First stab at #29153 range check in as-casts. Mostly working.

Having an issue forwarding Symbol::InTypeCheck into dart and back;
without it, the exceptions that are thrown are `TypeError`s and not
`CastError`s.

Certainly a bit longform to read, as well.

Committing for feedback/suggestions/help

BUG=
R=rmacnak@google.com

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

[mfairhurst, 2017-04-07 21:02:06]

mfairhurst@google.com changed reviewers:
+ rmacnak@google.com

[mfairhurst, 2017-04-07 21:02:07]

Hey Ryan!

I have an almost working proof of concept of using range checks in as-casts for
performance.

Originally I attempted to extend the CFG directly, which was a great chance to
learn the IR in more detail, but ultimately abandoned it when it was clear it
would be very verbose, and I couldn't get the use lists balanced.

So I moved on to writing the range check/throw in dart and always inline it,
which is a nice abstraction, and how its done by the existing cid range checks.

I have all but one of the cast tests passing. It seems like the problem is that
_TypeError._throwNew expects to get Symbol::InTypeCheck in order to throw a
CastError instead of a TypeError, a case I can't seem to trigger.

Looking for suggestions there!

Inside Exception::createAndThrowTypeError, it checks that argument 4.raw()
matches Symbol::InTypeCheck().raw(), which I thought I would be doing.

I might rewrite these as native, but then it can't be inlined, so could use some
advice.

[rmacnak, 2017-04-10 21:07:18]

On 2017/04/07 21:02:07, mfairhurst wrote:
> Hey Ryan!
> 
> I have an almost working proof of concept of using range checks in as-casts
for
> performance.
> 
> Originally I attempted to extend the CFG directly, which was a great chance to
> learn the IR in more detail, but ultimately abandoned it when it was clear it
> would be very verbose, and I couldn't get the use lists balanced.
> 
> So I moved on to writing the range check/throw in dart and always inline it,
> which is a nice abstraction, and how its done by the existing cid range
checks.

I also initially tried to construct the CFG directly for the
type-tests-as-cid-range-check, but Florian and I never managed to figured out
what was wrong with the graph.

> 
> I have all but one of the cast tests passing. It seems like the problem is
that
> _TypeError._throwNew expects to get Symbol::InTypeCheck in order to throw a
> CastError instead of a TypeError, a case I can't seem to trigger.
> 
> Looking for suggestions there!

exceptions.cc is also checking that the bound error is null, but you're passing
the empty string

> 
> Inside Exception::createAndThrowTypeError, it checks that argument 4.raw()
> matches Symbol::InTypeCheck().raw(), which I thought I would be doing.
> 
> I might rewrite these as native, but then it can't be inlined, so could use
some
> advice.

It might be worth splitting of a CastError._throwNew from TypeError._throwNew. I
don't know if there are cases where the exception isn't statically known, but I
would hope that isn't the case.

[rmacnak, 2017-04-10 21:07:24]

https://codereview.chromium.org/2805903004/diff/1/runtime/lib/errors_patch.dart
File runtime/lib/errors_patch.dart (right):

https://codereview.chromium.org/2805903004/diff/1/runtime/lib/errors_patch.da...
runtime/lib/errors_patch.dart:402: _TypeError._throwNew(0, instance, type,
InTypeCast, "");
0 -> position.

It's safe to say "in type type" here. The string literal will be canonicalized
to the same object as the predefined symbol.

https://codereview.chromium.org/2805903004/diff/1/runtime/vm/aot_optimizer.cc
File runtime/vm/aot_optimizer.cc (right):

https://codereview.chromium.org/2805903004/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:1574: flow_graph()->GetConstant(Smi::Handle(Z,
Smi::New(type_cid)));
ZoneHandle

https://codereview.chromium.org/2805903004/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:1576: Smi::Handle(Z,
Smi::New(call->token_pos().Pos())));
ZoneHandle

https://codereview.chromium.org/2805903004/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:1584: String::Handle(Z,
Symbols::InTypeCast().raw()))));
In general, this should be String::ZoneHandle. ZoneHandles out-last a
HandleScope, so any handles that will make it across the parser to the optimizer
or across the optimizer to the backend should be zone handles.  The handle one
gets from String::Handle is called a scope handle.

Because this is a pre-defined symbol, it already has a read-only handle, and you
can just say 

  arg = new (Z) PushArgumentInstr(new (Z) Value(flow_graph()->GetConstant(
     Symbols::InTypeCast()))));

https://codereview.chromium.org/2805903004/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:1636: flow_graph()->GetConstant(Smi::Handle(Z,
Smi::New(lower_limit)));
ZoneHandle

https://codereview.chromium.org/2805903004/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:1638: flow_graph()->GetConstant(Smi::Handle(Z,
Smi::New(upper_limit)));
ZoneHandle

https://codereview.chromium.org/2805903004/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:1648: String::Handle(Z,
Symbols::InTypeCast().raw()))));
ditto

[mfairhurst, 2017-04-11 01:49:50]

Ah, yes, the null error bound. For some reason had thought it was supposed to be
empty string, and was focused on that "in type check" part.

Followed your advice, all language/cast_tests now pass, thanks :)

I didn't end up creating _CastError._throwNew, as everything worked with it. I
could still see it being a good thing to create. Let me know if you want me to
do that too!

[mfairhurst, 2017-04-13 16:09:59]

Confirmed no regressions in the test suite:

./tools/test.py -mrelease -ax64 -rdart_precompiled -cprecompiler language
Test configuration: precompiler_dart_precompiled_release_x64
[1543:33 | 100% | + 4319 | -    0]

Takes a while to run, doesn't it!

[siva, 2017-04-13 17:01:03]

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

[siva, 2017-04-13 17:01:04]

https://codereview.chromium.org/2805903004/diff/20001/runtime/vm/aot_optimize...
File runtime/vm/aot_optimizer.cc (right):

https://codereview.chromium.org/2805903004/diff/20001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:1616: copy->DeepCopyTo(Z, new_call);
Maybe not for this CL but in a follow up CL, this pattern of looking up and
constructing the Static call seems to be repeated multiple times, it might make
sense to abstract this out into a helper method.

[mfairhurst, 2017-04-13 17:15:47]

On 2017/04/13 17:01:04, siva wrote:
>
https://codereview.chromium.org/2805903004/diff/20001/runtime/vm/aot_optimize...
> File runtime/vm/aot_optimizer.cc (right):
> 
>
https://codereview.chromium.org/2805903004/diff/20001/runtime/vm/aot_optimize...
> runtime/vm/aot_optimizer.cc:1616: copy->DeepCopyTo(Z, new_call);
> Maybe not for this CL but in a follow up CL, this pattern of looking up and
> constructing the Static call seems to be repeated multiple times, it might
make
> sense to abstract this out into a helper method.

Agreed. Originally I had abstracted the code for generating _cidRangeCheck into
a separate
method. That was where I then trying to add in branches to check null and throw
otherwise, by
checking the result of what was the same set of static calls. By going with the
all-in-dart
approach, it became a new set of methods with different parameters, and
abstraction is less
clear. It works, and doesn't require CFG manipulation, but also doesn't
currently contain any
refactoring of reused patterns.

Could definitely use something, hardest part would be coming up with the right
name for what
the common behavior truly is! :)

Would happily submit such a CL, and happily include it in this fix too if that's
preferred.

[mfairhurst, 2017-04-13 17:21:44]

Description was changed from

==========
First stab at #29153 range check in as-casts. Mostly working.

Having an issue forwarding Symbol::InTypeCheck into dart and back;
without it, the exceptions that are thrown are `TypeError`s and not
`CastError`s.

Certainly a bit longform to read, as well.

Committing for feedback/suggestions/help

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

to

==========
First stab at #29153 range check in as-casts. Mostly working.

Having an issue forwarding Symbol::InTypeCheck into dart and back;
without it, the exceptions that are thrown are `TypeError`s and not
`CastError`s.

Certainly a bit longform to read, as well.

Committing for feedback/suggestions/help

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

[siva, 2017-04-13 17:36:59]

Don't have to include it in this fix, a new CL is fine.

> Would happily submit such a CL, and happily include it in this fix too if
that's
> preferred.

[rmacnak, 2017-04-13 17:38:39]

On 2017/04/13 16:09:59, mfairhurst wrote:
> Confirmed no regressions in the test suite:
> 
> ./tools/test.py -mrelease -ax64 -rdart_precompiled -cprecompiler language
> Test configuration: precompiler_dart_precompiled_release_x64
> [1543:33 | 100% | + 4319 | -    0]
> 
> Takes a while to run, doesn't it!

How many cores do you have? For me, this takes about a hour, not a day. There's
another flag to the test harness '--use-blobs' which should slightly speed
things up by avoiding a step invoking gcc to build a dynamic library.

LGTM

[mfairhurst, 2017-04-13 17:45:02]

Description was changed from

==========
First stab at #29153 range check in as-casts. Mostly working.

Having an issue forwarding Symbol::InTypeCheck into dart and back;
without it, the exceptions that are thrown are `TypeError`s and not
`CastError`s.

Certainly a bit longform to read, as well.

Committing for feedback/suggestions/help

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

to

==========
First stab at #29153 range check in as-casts. Mostly working.

Having an issue forwarding Symbol::InTypeCheck into dart and back;
without it, the exceptions that are thrown are `TypeError`s and not
`CastError`s.

Certainly a bit longform to read, as well.

Committing for feedback/suggestions/help

BUG=
R=rmacnak@google.com

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

[mfairhurst, 2017-04-13 17:45:04]

Committed patchset #2 (id:20001) manually as
bffe47e678578f852dd19cddf919d62fd8da7fad (presubmit successful).