dart2js: Allow 'throw' when inlining

dart2js: Allow 'throw' when inlining

R=karlklose@google.com
BUG=

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

[karlklose, 2013-04-09 11:58:46]

LGTM.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:588: return
JS("String|Null", r"(#['$name'] || #)", f, null);
Why did you change this?

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:818: bool isReachable
= true;
The comment does not really match the name of the field.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2073: if
(!isReachable) return;
Would it be enough to abort in visitBlock instead of testing it for each
statement type?

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

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2023: class
HThrowExpression extends HInstruction {
Shouldn't this extends HControlFlow?

[ngeoffray, 2013-04-10 12:06:38]

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:617: $throw(ex) {
We should rename this method. It does not throw.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:639: * is not
allowed.
Please also add that a helper is never inlined. If it was to be inlined, we
could not do this trick.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/math_patch.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/math_patch.dart:8: patch double
sqrt(num x) {
Why do you need to change these?

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:814: * return further
up.  If this is true, then [_current] may be null.  If it
Missing ending paren after 'further up'.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1793: if
(isReachable) {
It looks weird to check this here while the code line 1778 makes you think it
can never happen.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3933: if
(isReachable) {
I can't see why this can happen.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2009:
world.registerStaticUse(nonThrowingHelper);
the compilation of throwExpression will figure that out, so no need to do it
explicitly.

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

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2023: class
HThrowExpression extends HInstruction {
Wouldn't it be simpler to have HTrow be treated as an expression in our SSA
nodes?

[erikcorry, 2013-04-10 12:52:32]

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:588: return
JS("String|Null", r"(#['$name'] || #)", f, null);
On 2013/04/09 11:58:46, karlklose wrote:
> Why did you change this?

When the inliner gets more agressive it can propagate a constant like '1' right
into the JS, so it says ".$name", which is a syntax error.  There are guards to
ensure we never try to execute that, but having a syntax error prevents the
program from even starting.

I am working on using the mini JS parser for these, so that the issue can be
dealt with better.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:617: $throw(ex) {
On 2013/04/10 12:06:39, ngeoffray wrote:
> We should rename this method. It does not throw.

Renamed to wrapException

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:639: * is not
allowed.
On 2013/04/10 12:06:39, ngeoffray wrote:
> Please also add that a helper is never inlined. If it was to be inlined, we
> could not do this trick.

Done.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/math_patch.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/math_patch.dart:8: patch double
sqrt(num x) {
On 2013/04/10 12:06:39, ngeoffray wrote:
> Why do you need to change these?

In this form the checkNum disappears completely when we inline and we happen to
know that it's a number.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:814: * return further
up.  If this is true, then [_current] may be null.  If it
On 2013/04/10 12:06:39, ngeoffray wrote:
> Missing ending paren after 'further up'.

Done.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:818: bool isReachable
= true;
On 2013/04/09 11:58:46, karlklose wrote:
> The comment does not really match the name of the field.

Done.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1793: if
(isReachable) {
On 2013/04/10 12:06:39, ngeoffray wrote:
> It looks weird to check this here while the code line 1778 makes you think it
> can never happen.

I replaced all these checks with asserts and it turns out that there is at least
one place in our tests where they are hit, so I changed it back.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2073: if
(!isReachable) return;
On 2013/04/09 11:58:46, karlklose wrote:
> Would it be enough to abort in visitBlock instead of testing it for each
> statement type?

I tried replacing this if with an assert and it was hit.  It seems harmless to
leave it as it is.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3933: if
(isReachable) {
On 2013/04/10 12:06:39, ngeoffray wrote:
> I can't see why this can happen.

Perhaps if there is more than one throw in an inlined method?

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2009:
world.registerStaticUse(nonThrowingHelper);
On 2013/04/10 12:06:39, ngeoffray wrote:
> the compilation of throwExpression will figure that out, so no need to do it
> explicitly.

Done.

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

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2023: class
HThrowExpression extends HInstruction {
On 2013/04/10 12:06:39, ngeoffray wrote:
> Wouldn't it be simpler to have HTrow be treated as an expression in our SSA
> nodes?

I don't think so.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2023: class
HThrowExpression extends HInstruction {
On 2013/04/09 11:58:46, karlklose wrote:
> Shouldn't this extends HControlFlow?

No, we can't do control flow (closing the block with a throw) inside an
expression because the code generator can't cope.  So we just throw and keep
going (rather like a call to a method that always throws).  But we stop
generating code at the next statement boundary.

If it turns out to be common and cause bloat, we can do the work to handle throw
properly in mid-statement.  I had a patch that worked for 99% of cases, but I
kept finding more corner cases.  It is possible that it would work now that I
fixed https://codereview.chromium.org/13723002/ but I sort of lost patience with
it after being at (what I thought was) 99% for a week or two.

[erikcorry, 2013-04-10 12:53:20]

That should read:

propagate a constant like '1' right into the JS, so it says "1.$name"

[ngeoffray, 2013-04-10 13:02:58]

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/math_patch.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/math_patch.dart:8: patch double
sqrt(num x) {
On 2013/04/10 12:52:32, erikcorry wrote:
> On 2013/04/10 12:06:39, ngeoffray wrote:
> > Why do you need to change these?
> 
> In this form the checkNum disappears completely when we inline and we happen
to
> know that it's a number.

Do you know why not in the other form?

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1793: if
(isReachable) {
On 2013/04/10 12:52:32, erikcorry wrote:
> On 2013/04/10 12:06:39, ngeoffray wrote:
> > It looks weird to check this here while the code line 1778 makes you think
it
> > can never happen.
> 
> I replaced all these checks with asserts and it turns out that there is at
least
> one place in our tests where they are hit, so I changed it back.

Isn't that a bug if that assert is hit? Do you know what test(s) failed?

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3933: if
(isReachable) {
On 2013/04/10 12:52:32, erikcorry wrote:
> On 2013/04/10 12:06:39, ngeoffray wrote:
> > I can't see why this can happen.
> 
> Perhaps if there is more than one throw in an inlined method?

But the first throw would abort visiting the block.

[erikcorry, 2013-04-11 09:09:35]

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/math_patch.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/math_patch.dart:8: patch double
sqrt(num x) {
On 2013/04/10 13:02:58, ngeoffray wrote:
> On 2013/04/10 12:52:32, erikcorry wrote:
> > On 2013/04/10 12:06:39, ngeoffray wrote:
> > > Why do you need to change these?
> > 
> > In this form the checkNum disappears completely when we inline and we happen
> to
> > know that it's a number.
> 
> Do you know why not in the other form?

Actually with the current version of the inlining visitor we can inline it
either way and the code does not change.  I reverted the changes to this file.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1793: if
(isReachable) {
On 2013/04/10 13:02:58, ngeoffray wrote:
> On 2013/04/10 12:52:32, erikcorry wrote:
> > On 2013/04/10 12:06:39, ngeoffray wrote:
> > > It looks weird to check this here while the code line 1778 makes you think
> it
> > > can never happen.
> > 
> > I replaced all these checks with asserts and it turns out that there is at
> least
> > one place in our tests where they are hit, so I changed it back.
> 
> Isn't that a bug if that assert is hit? Do you know what test(s) failed?

It's the inlined throw test that I just introduced.  It's a ternary expression
where one of the 3 values threw, and one of the other branches is an inlined
function.  When we enter the visitBlock from the inlined method the isReachable
flag is already false before the first statement visited, and we don't test the
flag until after the first statement is visited.

I fixed visitBlock to test the flag before the first statement is visited.  The
net effect is the same as having these if's, which have now been replaced with
asserts.

https://codereview.chromium.org/13947004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3933: if
(isReachable) {
On 2013/04/10 13:02:58, ngeoffray wrote:
> On 2013/04/10 12:52:32, erikcorry wrote:
> > On 2013/04/10 12:06:39, ngeoffray wrote:
> > > I can't see why this can happen.
> > 
> > Perhaps if there is more than one throw in an inlined method?
> 
> But the first throw would abort visiting the block.

I couldn't make this trigger so I removed it.

[erikcorry, 2013-04-11 09:20:47]

Committed patchset #3 manually as r21259 (presubmit successful).

[ngeoffray, 2013-04-11 09:31:08]

LGTM!

https://codereview.chromium.org/13947004/diff/14001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/13947004/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:637: * This wraos
the exception and actually does the throw too.  It is
wraos -> wraps

https://codereview.chromium.org/13947004/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:637: * This wraos
the exception and actually does the throw too.  It is
actually does the throw too -> does the throw

https://codereview.chromium.org/13947004/diff/14001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/13947004/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1775: if
(!isReachable) return;  // This can happen when inlining.
// This only happens when inlining

Can you add: assert(current != null) ?

https://codereview.chromium.org/13947004/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3906: if
(node.expression == null || !isReachable) {
I think you can remove the isReachable check.

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

https://codereview.chromium.org/13947004/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2025: toString() =>
'throwexpression';
throwexpression -> throw expression