Allow Crankshaft to tolerate certain do-expressions

Allow Crankshaft to tolerate certain do-expressions

Crankshaft can't track operand/environment changes between arbitrary statements.
We need that to fully support do-expressions. Instead, a subset is supported
by bailing out on break statements, continue statements, and if we've made an
OSR entry within a do-expression.

This partial support is a good idea because do-expressions are a useful tool
for desugaring during parsing.

BUG=
R=mstarzinger@chromium.org

Committed: https://crrev.com/6783854603cf3c4fdfa759f6a3d2fdbf34583623
Cr-Commit-Position: refs/heads/master@{#34491}

[mvstanton, 2016-03-04 09:25:25]

Hi Michael,
Here is the do-expression work we discussed, thanks!
--Mike

[Michael Starzinger, 2016-03-04 09:37:28]

https://codereview.chromium.org/1769463002/diff/1/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/1769463002/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:4057: do_expression_scope_count_ =
outer_->DoExpressionScopeCount();
Shouldn't the counter start at 0 here? Otherwise the case where we inline a
"normal" continue statement inside a do-expression will still bailout. I am
thinking about the following test case (assuming g is inlined into f) ...

function g() { while (true) { break } }
function f() { 1 + do { g() } }

https://codereview.chromium.org/1769463002/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:4842: return Bailout(kDoExpression);
nit: This bailout reads "Do expression encountered", can we have a more accurate
bailout reason for the cases where we actually bailout because of control flow
that potentially crosses do-expression boundary? Or alternatively a one-line
comment here explaining the reason for this bailout.

https://codereview.chromium.org/1769463002/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:4879: return Bailout(kDoExpression);
nit: Likewise.

https://codereview.chromium.org/1769463002/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:5546: VisitVariableProxy(expr->result());
nit: Just "Visit" here, otherwise you won't get stack overflow checking.

[mvstanton, 2016-03-04 10:02:53]

mvstanton@chromium.org changed reviewers:
+ bmeurer@chromium.org

[mvstanton, 2016-03-04 10:02:53]

Hi, addressed comments,
And Benedikt, can you offer a review please?
Thanks,
--Mike

https://codereview.chromium.org/1769463002/diff/1/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/1769463002/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:4057: do_expression_scope_count_ =
outer_->DoExpressionScopeCount();
On 2016/03/04 09:37:28, Michael Starzinger wrote:
> Shouldn't the counter start at 0 here? Otherwise the case where we inline a
> "normal" continue statement inside a do-expression will still bailout. I am
> thinking about the following test case (assuming g is inlined into f) ...
> 
> function g() { while (true) { break } }
> function f() { 1 + do { g() } }

Good point, thanks! Done.

[mvstanton, 2016-03-04 10:05:29]

Missed a few comments, uploaded a new patch.

https://codereview.chromium.org/1769463002/diff/1/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/1769463002/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:4842: return Bailout(kDoExpression);
On 2016/03/04 09:37:28, Michael Starzinger wrote:
> nit: This bailout reads "Do expression encountered", can we have a more
accurate
> bailout reason for the cases where we actually bailout because of control flow
> that potentially crosses do-expression boundary? Or alternatively a one-line
> comment here explaining the reason for this bailout.

Done.

https://codereview.chromium.org/1769463002/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:4879: return Bailout(kDoExpression);
On 2016/03/04 09:37:28, Michael Starzinger wrote:
> nit: Likewise.

Done.

https://codereview.chromium.org/1769463002/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:5546: VisitVariableProxy(expr->result());
On 2016/03/04 09:37:28, Michael Starzinger wrote:
> nit: Just "Visit" here, otherwise you won't get stack overflow checking.

Done.

[Michael Starzinger, 2016-03-04 10:08:06]

LGTM.

[Benedikt Meurer, 2016-03-04 10:59:58]

LGTM (rubber-stamped)

[mvstanton, 2016-03-04 11:48:02]

The CQ bit was checked by mvstanton@chromium.org

[commit-bot: I haz the power, 2016-03-04 11:48:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1769463002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1769463002/40001

[commit-bot: I haz the power, 2016-03-04 11:50:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-04 11:51:58]

Description was changed from

==========
Allow Crankshaft to tolerate certain do-expressions

Crankshaft can't track operand/environment changes between arbitrary statements.
We need that to fully support do-expressions. Instead, a subset is supported
by bailing out on break statements, continue statements, and if we've made an
OSR entry within a do-expression.

This partial support is a good idea because do-expressions are a useful tool
for desugaring during parsing.

BUG=
R=mstarzinger@chromium.org
==========

to

==========
Allow Crankshaft to tolerate certain do-expressions

Crankshaft can't track operand/environment changes between arbitrary statements.
We need that to fully support do-expressions. Instead, a subset is supported
by bailing out on break statements, continue statements, and if we've made an
OSR entry within a do-expression.

This partial support is a good idea because do-expressions are a useful tool
for desugaring during parsing.

BUG=
R=mstarzinger@chromium.org

Committed: https://crrev.com/6783854603cf3c4fdfa759f6a3d2fdbf34583623
Cr-Commit-Position: refs/heads/master@{#34491}
==========

[commit-bot: I haz the power, 2016-03-04 11:51:59]

Patchset 3 (id:??) landed as
https://crrev.com/6783854603cf3c4fdfa759f6a3d2fdbf34583623
Cr-Commit-Position: refs/heads/master@{#34491}