Throw NoSuchMethod or ArgumentError instead of generating a bailout, when we know the next instruct…

Throw NoSuchMethod or ArgumentError instead of generating a bailout, when we know the next instruction after a type guard will throw.

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

[kasperl, 2013-04-23 06:12:43]

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/bailout.dart (right):

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:194: // Returns
whether an invcation of [selector] on [receiver] will throw a
invcation -> invocation

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:247: // Find out if
we should actually just emit a check for it. If
It would be nice if this could be refactored into a bunch of nicer helper
methods with good names.

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:268: for
(HInstruction user in instruction.usedBy) {
Somehow it feels like we could use a better strategy here. If we inserted
throwing type guards in front of all throwing users we wouldn't need the
same-block restriction. We could always work on moving the throwing guards up
the dominator tree and eliminating redundant ones later (maybe that could be
done in general for type guards so we wouldn't have to insert them so early?).

Wouldn't that also eliminate the rather weird distinction between firstUser and
firstUserWithSelector?

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:289: assert(firstUser
== firstUserWithSelector || !firstUser.hasSideEffects());
Long line.

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1571: Selector
selector;
final?

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/types_propagation.dart
(right):

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/types_propagation.dart:190: if
(specializer is BinaryArithmeticSpecializer
Add a method on the specializer?

[sra1, 2013-04-23 06:53:20]

in swarm I see:

        len = iterable.get$length(iterable);
        if (typeof len !== "number")
          return this.addAll$1$bailout1(1, iterable, len, t1);
        i = 0;
        for (; i < len; ++i)
          t1.appendChild(iterable.$index(iterable, 0));
-->
        len = iterable.get$length(iterable);
        i = 0;
        while (true) {
          if (typeof len !== "number")
            throw $.iae(len);
          if (!(i < len))
            break;
          t1.appendChild(iterable.$index(iterable, 0));
          ++i;
        }

it looks like a performance regression to check "len" each time in the loop.

Also, we should not be shy of generating "for(;;){...}" and
"for(init;;inc){...}"

[ngeoffray, 2013-04-23 08:06:04]

Thanks Kasper, PTAL.

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/bailout.dart (right):

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:194: // Returns
whether an invcation of [selector] on [receiver] will throw a
On 2013/04/23 06:12:43, kasperl wrote:
> invcation -> invocation

Done.

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:247: // Find out if
we should actually just emit a check for it. If
On 2013/04/23 06:12:43, kasperl wrote:
> It would be nice if this could be refactored into a bunch of nicer helper
> methods with good names.

Done.

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:268: for
(HInstruction user in instruction.usedBy) {
On 2013/04/23 06:12:43, kasperl wrote:
> Somehow it feels like we could use a better strategy here. If we inserted
> throwing type guards in front of all throwing users we wouldn't need the
> same-block restriction. We could always work on moving the throwing guards up
> the dominator tree and eliminating redundant ones later (maybe that could be
> done in general for type guards so we wouldn't have to insert them so early?).

Agree. After discussing a bit about it, I'll work on another CL that will put
type guards at relevant use site, and moving them up when needed.

> 
> Wouldn't that also eliminate the rather weird distinction between firstUser
and
> firstUserWithSelector?

Actually, I don't need this distinction, I just changed the code to update all
users after the first user that has a selector.

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:289: assert(firstUser
== firstUserWithSelector || !firstUser.hasSideEffects());
On 2013/04/23 06:12:43, kasperl wrote:
> Long line.

Done.

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1571: Selector
selector;
On 2013/04/23 06:12:43, kasperl wrote:
> final?

Done.

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/types_propagation.dart
(right):

https://codereview.chromium.org/14404004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/types_propagation.dart:190: if
(specializer is BinaryArithmeticSpecializer
On 2013/04/23 06:12:43, kasperl wrote:
> Add a method on the specializer?

Done.

[kasperl, 2013-04-23 09:01:43]

LGTM.

https://codereview.chromium.org/14404004/diff/11003/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/bailout.dart (right):

https://codereview.chromium.org/14404004/diff/11003/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:233: // (meaning it
will be a call), we can put a type check instead
This comment seems a little incomplete.

https://codereview.chromium.org/14404004/diff/11003/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:325: HTypeGuard check
= new HTypeGuard(speculativeType, instruction, target);
Undo this change?

[ngeoffray, 2013-04-23 11:00:56]

Thanks Kasper. I also added a new heuristic in typeGuardWouldBeValuable to not
insert a type guard if it is incompatible with a user with a selector.

https://codereview.chromium.org/14404004/diff/11003/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/bailout.dart (right):

https://codereview.chromium.org/14404004/diff/11003/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:233: // (meaning it
will be a call), we can put a type check instead
On 2013/04/23 09:01:43, kasperl wrote:
> This comment seems a little incomplete.

Wrong copy/paste. Removed the line.

https://codereview.chromium.org/14404004/diff/11003/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:325: HTypeGuard check
= new HTypeGuard(speculativeType, instruction, target);
On 2013/04/23 09:01:43, kasperl wrote:
> Undo this change?

Done.

[ngeoffray, 2013-04-23 11:52:01]

Committed patchset #5 manually as r21870 (presubmit successful).