[fullcode] Change fullcode to compile finally using the token approach.

[fullcode] Change fullcode to compile finally using the token approach.

This change should unify handling of finally blocks in Turbofan's
AstGraphBuilder and in full-code. This should enable smooth deoptimization
from finally blocks.

Committed: https://crrev.com/334d17946ccbca4e8b96c6211a3387000fe63d55
Cr-Commit-Position: refs/heads/master@{#33780}

[Jarin, 2016-02-05 05:33:54]

Description was changed from

==========
[fullcode] Change fullcode to compile finally using the token approach.

This change should unify handling of finally blocks in Turbofan's
AstGraphBuilder and in full-code. This should enable smooth deoptimization
from finally blocks.
==========

to

==========
[fullcode] Change fullcode to compile finally using the token approach.

This change should unify handling of finally blocks in Turbofan's
AstGraphBuilder and in full-code. This should enable smooth deoptimization
from finally blocks.
==========

[Jarin, 2016-02-05 05:33:54]

jarin@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Jarin, 2016-02-05 09:30:59]

Could you take a look, please?

The main difference from the previous version is the token dispenser for fall
through and emitting the return sequence (return sequence is skipped if we go to
the deferred return via the finally block).

[Michael Starzinger, 2016-02-05 13:44:58]

LGTM. Nice stuff. Only nits.

https://codereview.chromium.org/1663323003/diff/60001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1663323003/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:277: Node* WrapToken(int token_id) {
nit: Can we call this "NewPathToken" instead? Because it really creates a new
node.

https://codereview.chromium.org/1663323003/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:281: return owner_->jsgraph()->Constant(
nit: "return WrapToken(TokenDispenserForFinally::kFallThroughToken);"

https://codereview.chromium.org/1663323003/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1513: environment()->Push(token);  //
TODO(mstarzinger): Cook token!
nit: Can we drop this TODO?

https://codereview.chromium.org/1663323003/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1529: token = environment()->Pop();  //
TODO(mstarzinger): Uncook token!
nit: Can we drop this TODO?

https://codereview.chromium.org/1663323003/diff/60001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/1663323003/diff/60001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:866: if (current->IsTryFinally()) {
suggestion: Does it make sense to record a code comment here?
Comment cmnt(masm(), "[ Deferred continue through finally");

https://codereview.chromium.org/1663323003/diff/60001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:904: if (current->IsTryFinally()) {
suggestion: Does it make sense to record a code comment here?
Comment cmnt(masm(), "[ Deferred break through finally");

[Jarin, 2016-02-05 13:55:08]

Thanks for the suggestions!

https://codereview.chromium.org/1663323003/diff/60001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1663323003/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:277: Node* WrapToken(int token_id) {
On 2016/02/05 13:44:57, Michael Starzinger wrote:
> nit: Can we call this "NewPathToken" instead? Because it really creates a new
> node.

Done.

https://codereview.chromium.org/1663323003/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:281: return owner_->jsgraph()->Constant(
On 2016/02/05 13:44:57, Michael Starzinger wrote:
> nit: "return WrapToken(TokenDispenserForFinally::kFallThroughToken);"

Done.

https://codereview.chromium.org/1663323003/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1513: environment()->Push(token);  //
TODO(mstarzinger): Cook token!
On 2016/02/05 13:44:58, Michael Starzinger wrote:
> nit: Can we drop this TODO?

Done.

https://codereview.chromium.org/1663323003/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1529: token = environment()->Pop();  //
TODO(mstarzinger): Uncook token!
On 2016/02/05 13:44:57, Michael Starzinger wrote:
> nit: Can we drop this TODO?

Done.

https://codereview.chromium.org/1663323003/diff/60001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/1663323003/diff/60001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:866: if (current->IsTryFinally()) {
On 2016/02/05 13:44:58, Michael Starzinger wrote:
> suggestion: Does it make sense to record a code comment here?
> Comment cmnt(masm(), "[ Deferred continue through finally");

Done.

https://codereview.chromium.org/1663323003/diff/60001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:904: if (current->IsTryFinally()) {
On 2016/02/05 13:44:58, Michael Starzinger wrote:
> suggestion: Does it make sense to record a code comment here?
> Comment cmnt(masm(), "[ Deferred break through finally");

Done.

[Jarin, 2016-02-05 13:56:19]

The CQ bit was checked by jarin@chromium.org

[Jarin, 2016-02-05 13:56:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1663323003/#ps80001
(title: "Address review comments")

[commit-bot: I haz the power, 2016-02-05 13:56:32]

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

[commit-bot: I haz the power, 2016-02-05 14:14:48]

Description was changed from

==========
[fullcode] Change fullcode to compile finally using the token approach.

This change should unify handling of finally blocks in Turbofan's
AstGraphBuilder and in full-code. This should enable smooth deoptimization
from finally blocks.
==========

to

==========
[fullcode] Change fullcode to compile finally using the token approach.

This change should unify handling of finally blocks in Turbofan's
AstGraphBuilder and in full-code. This should enable smooth deoptimization
from finally blocks.
==========

[commit-bot: I haz the power, 2016-02-05 14:14:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-05 14:15:37]

Description was changed from

==========
[fullcode] Change fullcode to compile finally using the token approach.

This change should unify handling of finally blocks in Turbofan's
AstGraphBuilder and in full-code. This should enable smooth deoptimization
from finally blocks.
==========

to

==========
[fullcode] Change fullcode to compile finally using the token approach.

This change should unify handling of finally blocks in Turbofan's
AstGraphBuilder and in full-code. This should enable smooth deoptimization
from finally blocks.

Committed: https://crrev.com/334d17946ccbca4e8b96c6211a3387000fe63d55
Cr-Commit-Position: refs/heads/master@{#33780}
==========

[commit-bot: I haz the power, 2016-02-05 14:15:38]

Patchset 5 (id:??) landed as
https://crrev.com/334d17946ccbca4e8b96c6211a3387000fe63d55
Cr-Commit-Position: refs/heads/master@{#33780}