First stab at try-catch and try-finally in TurboFan.

First stab at try-catch and try-finally in TurboFan.

R=titzer@chromium.org,jarin@chromium.org
TEST=cctest/test-run-jsexceptions

Committed: https://crrev.com/77d612691d79b7c9c8dbc6936b406d1dd4a4ba2b
Cr-Commit-Position: refs/heads/master@{#26398}

[Michael Starzinger, 2015-01-30 14:30:27]

Note that this only implements local exceptional control-flow (as in 'throw' in
the same function as the 'try'). Implicit exceptional control-flow from calls is
not covered.

Also there is still one issue with bailout-ids I still need to address before
being able to land this. But please feel free to shoot a first round of comments
my way while I work on that.

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:2570: // TODO(mstarzinger): Thread through the
correct bailout id to this point.
This is the TODO I still need to address.

[titzer, 2015-02-02 09:47:29]

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:351: return
owner_->NewNode(owner_->javascript()->StrictEqual(), t1, t2);
I think you want pointer/integer equality here. JS operators are for expressing
source program computations, not internal computations.

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:42: class ContextScope;
Do this class and its subclasses really need to be nested in the graph builder?

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:411: ControlScope(AstGraphBuilder* owner, int
drop_extra)
{owner} is a bad name. Would prefer the more verbose {ast_graph_builder} (and
hopefully in the future simply {graph_builder}) to make this more clear.

{drop_extra} should be named {expr_stack_delta} or something similar to make it
more clear.

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:439: void PerformCommand(Command cmd,
Statement* stmt, Node* value);
{stmt} is called {target} in all the subclasses.

https://codereview.chromium.org/873423004/diff/1/src/compiler/graph-builder.h
File src/compiler/graph-builder.h (right):

https://codereview.chromium.org/873423004/diff/1/src/compiler/graph-builder.h...
src/compiler/graph-builder.h:238: // Friends using the values vector.
What if instead of friending, you added public methods Push() and Pop()?

This will get easier when we put AstGraphBuilder and GraphBuilder back together
again.

https://codereview.chromium.org/873423004/diff/1/test/cctest/compiler/test-ru...
File test/cctest/compiler/test-run-jsexceptions.cc (right):

https://codereview.chromium.org/873423004/diff/1/test/cctest/compiler/test-ru...
test/cctest/compiler/test-run-jsexceptions.cc:48: TEST(Catch) {
+ 1

Good to have cctests for TurboFan, yay. Can we also have these tests as mjsunit?
Redundancy is better than holes in coverage :-)

Also, I foresee some quite nasty interactions with OSR in the future, so we'll
probably want to have lots of mjsunit tests that can be run in various modes.

[Michael Starzinger, 2015-02-02 14:47:41]

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:351: return
owner_->NewNode(owner_->javascript()->StrictEqual(), t1, t2);
On 2015/02/02 09:47:29, titzer wrote:
> I think you want pointer/integer equality here. JS operators are for
expressing
> source program computations, not internal computations.

Done.

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:42: class ContextScope;
On 2015/02/02 09:47:29, titzer wrote:
> Do this class and its subclasses really need to be nested in the graph
builder?

Acknowledged. As discussed offline, it gives access to protected members of
AstGraphBuilder. Otherwise all the ControlScope classes would either need to be
friends or we would need to make a ton of methods on AstGraphBuilder public.
Let's refactor this in one go for all nested classes later.

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:411: ControlScope(AstGraphBuilder* owner, int
drop_extra)
On 2015/02/02 09:47:29, titzer wrote:
> {owner} is a bad name. Would prefer the more verbose {ast_graph_builder} (and
> hopefully in the future simply {graph_builder}) to make this more clear.
> 
> {drop_extra} should be named {expr_stack_delta} or something similar to make
it
> more clear.

Done. As discussed offline, used {builder} instead of {owner} and {stack_delta}
instead of {drop_extra}

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:439: void PerformCommand(Command cmd,
Statement* stmt, Node* value);
On 2015/02/02 09:47:29, titzer wrote:
> {stmt} is called {target} in all the subclasses.

Done. Here and in the occurrences below.

https://codereview.chromium.org/873423004/diff/1/src/compiler/graph-builder.h
File src/compiler/graph-builder.h (right):

https://codereview.chromium.org/873423004/diff/1/src/compiler/graph-builder.h...
src/compiler/graph-builder.h:238: // Friends using the values vector.
On 2015/02/02 09:47:29, titzer wrote:
> What if instead of friending, you added public methods Push() and Pop()?
> 
> This will get easier when we put AstGraphBuilder and GraphBuilder back
together
> again.

Acknowledged. Yeah, well, we could do that if StructuredGraphBuilder weren't
abstractified out. I placed a TODO here.

https://codereview.chromium.org/873423004/diff/1/test/cctest/compiler/test-ru...
File test/cctest/compiler/test-run-jsexceptions.cc (right):

https://codereview.chromium.org/873423004/diff/1/test/cctest/compiler/test-ru...
test/cctest/compiler/test-run-jsexceptions.cc:48: TEST(Catch) {
On 2015/02/02 09:47:29, titzer wrote:
> + 1
> 
> Good to have cctests for TurboFan, yay. Can we also have these tests as
mjsunit?
> Redundancy is better than holes in coverage :-)
> 
> Also, I foresee some quite nasty interactions with OSR in the future, so we'll
> probably want to have lots of mjsunit tests that can be run in various modes.
> 

The cases covered by these cases are already covered by try.js in the mjsunit
test suite.

[Michael Starzinger, 2015-02-02 15:08:02]

Added workaround. PTAL.

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:2570: // TODO(mstarzinger): Thread through the
correct bailout id to this point.
On 2015/01/30 14:30:27, Michael Starzinger wrote:
> This is the TODO I still need to address.

I added a workaround that only uses the ControlScope to throw when
--turbo-exceptions is passed. This way I can solve the bailout-id separately,
because this one needs more thought than just threading through the ID.

[Michael Starzinger, 2015-02-02 17:08:42]

Had to re-iterate on one of your comments. PTAL.

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:351: return
owner_->NewNode(owner_->javascript()->StrictEqual(), t1, t2);
On 2015/02/02 14:47:41, Michael Starzinger wrote:
> On 2015/02/02 09:47:29, titzer wrote:
> > I think you want pointer/integer equality here. JS operators are for
> expressing
> > source program computations, not internal computations.
> 
> Done.

Yeah, well, ain't gonna happen as long as our Phi's have a machine
representation and are all marked as tagged. I left a TODO and verified, we
correctly lower to Word32Equal in the pipeline, so the code at the end should be
"optimal" already.

[titzer, 2015-02-03 08:56:14]

On 2015/02/02 14:47:41, Michael Starzinger wrote:
>
https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
> File src/compiler/ast-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
> src/compiler/ast-graph-builder.cc:351: return
> owner_->NewNode(owner_->javascript()->StrictEqual(), t1, t2);
> On 2015/02/02 09:47:29, titzer wrote:
> > I think you want pointer/integer equality here. JS operators are for
> expressing
> > source program computations, not internal computations.
> 
> Done.
> 
>
https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
> File src/compiler/ast-graph-builder.h (right):
> 
>
https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
> src/compiler/ast-graph-builder.h:42: class ContextScope;
> On 2015/02/02 09:47:29, titzer wrote:
> > Do this class and its subclasses really need to be nested in the graph
> builder?
> 
> Acknowledged. As discussed offline, it gives access to protected members of
> AstGraphBuilder. Otherwise all the ControlScope classes would either need to
be
> friends or we would need to make a ton of methods on AstGraphBuilder public.
> Let's refactor this in one go for all nested classes later.
> 
>
https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
> src/compiler/ast-graph-builder.h:411: ControlScope(AstGraphBuilder* owner, int
> drop_extra)
> On 2015/02/02 09:47:29, titzer wrote:
> > {owner} is a bad name. Would prefer the more verbose {ast_graph_builder}
(and
> > hopefully in the future simply {graph_builder}) to make this more clear.
> > 
> > {drop_extra} should be named {expr_stack_delta} or something similar to make
> it
> > more clear.
> 
> Done. As discussed offline, used {builder} instead of {owner} and
{stack_delta}
> instead of {drop_extra}
> 
>
https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
> src/compiler/ast-graph-builder.h:439: void PerformCommand(Command cmd,
> Statement* stmt, Node* value);
> On 2015/02/02 09:47:29, titzer wrote:
> > {stmt} is called {target} in all the subclasses.
> 
> Done. Here and in the occurrences below.
> 
> https://codereview.chromium.org/873423004/diff/1/src/compiler/graph-builder.h
> File src/compiler/graph-builder.h (right):
> 
>
https://codereview.chromium.org/873423004/diff/1/src/compiler/graph-builder.h...
> src/compiler/graph-builder.h:238: // Friends using the values vector.
> On 2015/02/02 09:47:29, titzer wrote:
> > What if instead of friending, you added public methods Push() and Pop()?
> > 
> > This will get easier when we put AstGraphBuilder and GraphBuilder back
> together
> > again.
> 
> Acknowledged. Yeah, well, we could do that if StructuredGraphBuilder weren't
> abstractified out. I placed a TODO here.
> 
>
https://codereview.chromium.org/873423004/diff/1/test/cctest/compiler/test-ru...
> File test/cctest/compiler/test-run-jsexceptions.cc (right):
> 
>
https://codereview.chromium.org/873423004/diff/1/test/cctest/compiler/test-ru...
> test/cctest/compiler/test-run-jsexceptions.cc:48: TEST(Catch) {
> On 2015/02/02 09:47:29, titzer wrote:
> > + 1
> > 
> > Good to have cctests for TurboFan, yay. Can we also have these tests as
> mjsunit?
> > Redundancy is better than holes in coverage :-)
> > 
> > Also, I foresee some quite nasty interactions with OSR in the future, so
we'll
> > probably want to have lots of mjsunit tests that can be run in various
modes.
> > 
> 
> The cases covered by these cases are already covered by try.js in the mjsunit
> test suite.

Ok, can you leave a TODO somewhere, since I think we will want to increase
coverage with some more mjsunit tests for try.

[titzer, 2015-02-03 08:56:33]

On 2015/02/02 17:08:42, Michael Starzinger wrote:
> Had to re-iterate on one of your comments. PTAL.
> 
>
https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
> File src/compiler/ast-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-build...
> src/compiler/ast-graph-builder.cc:351: return
> owner_->NewNode(owner_->javascript()->StrictEqual(), t1, t2);
> On 2015/02/02 14:47:41, Michael Starzinger wrote:
> > On 2015/02/02 09:47:29, titzer wrote:
> > > I think you want pointer/integer equality here. JS operators are for
> > expressing
> > > source program computations, not internal computations.
> > 
> > Done.
> 
> Yeah, well, ain't gonna happen as long as our Phi's have a machine
> representation and are all marked as tagged. I left a TODO and verified, we
> correctly lower to Word32Equal in the pipeline, so the code at the end should
be
> "optimal" already.

Ok.

[titzer, 2015-02-03 09:01:22]

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

https://codereview.chromium.org/873423004/diff/60001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:457: void
AstGraphBuilder::ControlScope::BreakTarget(BreakableStatement* stmt) {
The names {BreakTarget} and {ContinueTarget} have always been a little confusing
to me. Maybe {BreakTo} and {ContinueTo} would be clearer? Then it would read:

execution_control()->ContinueTo(stmt->target())

https://codereview.chromium.org/873423004/diff/60001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/873423004/diff/60001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:456: // Control scope implementation for a
BreakableStatement.
Is it necessary to put these full declarations in the header? If you move them
to the .cc file then they each can be fully self-contained.

[Michael Starzinger, 2015-02-03 09:18:53]

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

https://codereview.chromium.org/873423004/diff/60001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:457: void
AstGraphBuilder::ControlScope::BreakTarget(BreakableStatement* stmt) {
On 2015/02/03 09:01:22, titzer wrote:
> The names {BreakTarget} and {ContinueTarget} have always been a little
confusing
> to me. Maybe {BreakTo} and {ContinueTo} would be clearer? Then it would read:
> 
> execution_control()->ContinueTo(stmt->target())

Done.

https://codereview.chromium.org/873423004/diff/60001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/873423004/diff/60001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:456: // Control scope implementation for a
BreakableStatement.
On 2015/02/03 09:01:22, titzer wrote:
> Is it necessary to put these full declarations in the header? If you move them
> to the .cc file then they each can be fully self-contained.

Done.

[Michael Starzinger, 2015-02-03 09:35:18]

Rebased. PTAL.

https://codereview.chromium.org/873423004/diff/1/src/compiler/graph-builder.h
File src/compiler/graph-builder.h (right):

https://codereview.chromium.org/873423004/diff/1/src/compiler/graph-builder.h...
src/compiler/graph-builder.h:238: // Friends using the values vector.
On 2015/02/02 14:47:41, Michael Starzinger wrote:
> On 2015/02/02 09:47:29, titzer wrote:
> > What if instead of friending, you added public methods Push() and Pop()?
> > 
> > This will get easier when we put AstGraphBuilder and GraphBuilder back
> together
> > again.
> 
> Acknowledged. Yeah, well, we could do that if StructuredGraphBuilder weren't
> abstractified out. I placed a TODO here.

Done. Now possible after rebasing.

[Michael Starzinger, 2015-02-03 09:40:20]

Patchset #7 (id:120001) has been deleted

[titzer, 2015-02-03 09:50:13]

LGTM with comment

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

https://codereview.chromium.org/873423004/diff/140001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:86: virtual bool Execute(Command cmd,
Statement* target, Node* value) OVERRIDE;
Would be nice to have the bodies inline here :-)

[Michael Starzinger, 2015-02-03 09:59:21]

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

https://codereview.chromium.org/873423004/diff/140001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:86: virtual bool Execute(Command cmd,
Statement* target, Node* value) OVERRIDE;
On 2015/02/03 09:50:13, titzer wrote:
> Would be nice to have the bodies inline here :-)

Done. You slight preference is my command. :)

[Michael Starzinger, 2015-02-03 12:05:14]

The CQ bit was checked by mstarzinger@chromium.org

[commit-bot: I haz the power, 2015-02-03 12:06:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/873423004/160001

[commit-bot: I haz the power, 2015-02-03 13:09:50]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-02-03 13:10:12]

Patchset 8 (id:??) landed as
https://crrev.com/77d612691d79b7c9c8dbc6936b406d1dd4a4ba2b
Cr-Commit-Position: refs/heads/master@{#26398}