[turbofan] Also inline into try blocks.

[turbofan] Also inline into try blocks.

This removes test/webkit/fast/js/stack-overflow-arrity-catch.js, which tests that the stack overflows in a very particular way. It doesn't seem to test anything important, and only used to work because we didn't inline into try-blocks.

BUG=
R=jarin

Committed: https://crrev.com/791118fca523d4d868860be68f0a3d520a3a24cd
Cr-Commit-Position: refs/heads/master@{#38976}

[bgeron, 2016-08-05 12:18:14]

The CQ bit was checked by bgeron@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 12:18:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bgeron, 2016-08-05 12:23:40]

This PS still has all tests in one file. In Optdebug on my workstation, the test
takes

- without any flags, 6 seconds
- with --exhaustive-flags, about 3 minutes.

Earlier I measured ~20 seconds, but I think that was because I was running with
--trace-turbo.

[commit-bot: I haz the power, 2016-08-05 12:30:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 12:30:32]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Jarin, 2016-08-05 14:04:48]

Random comments.

https://codereview.chromium.org/2216353002/diff/1/src/compiler/js-inlining.cc
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/2216353002/diff/1/src/compiler/js-inlining.cc...
src/compiler/js-inlining.cc:146: const Operator* op = common()->IfException();
Nit: no need for separate variable, just inline into the NewNode call below.

https://codereview.chromium.org/2216353002/diff/1/src/compiler/js-inlining.cc...
src/compiler/js-inlining.cc:387: Node* exceptionTarget = nullptr;
exceptionTarget -> exception_target

https://codereview.chromium.org/2216353002/diff/1/src/compiler/js-inlining.cc...
src/compiler/js-inlining.cc:476: subcall->opcode() == IrOpcode::kJSCallRuntime)
{
As discussed offline, you need a more general way:

I suggest looking for nodes that do not have NoThrow property, and then taking
the ones that have an IfSuccess use, but no IfException use.

https://codereview.chromium.org/2216353002/diff/1/src/compiler/js-inlining.cc...
src/compiler/js-inlining.cc:482: s << "found exception node #" << use->id();
Remove the logging.

https://codereview.chromium.org/2216353002/diff/1/src/compiler/js-inlining.h
File src/compiler/js-inlining.h (right):

https://codereview.chromium.org/2216353002/diff/1/src/compiler/js-inlining.h#...
src/compiler/js-inlining.h:57: Node* exceptionTarget, NodeVector
uncaught_subcalls);
exceptionTarget -> exception_target

[bgeron, 2016-08-05 14:08:07]

The CQ bit was checked by bgeron@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 14:08:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bgeron, 2016-08-05 14:20:23]

The CQ bit was checked by bgeron@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 14:20:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 14:32:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 14:32:16]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[bgeron, 2016-08-08 09:05:23]

The CQ bit was checked by bgeron@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 09:05:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bgeron, 2016-08-08 09:07:46]

Description was changed from

==========
[turbofan] Also inline into try blocks.

BUG=
R=jarin
==========

to

==========
[turbofan] Also inline into try blocks.

This removes test/webkit/fast/js/stack-overflow-arrity-catch.js, which tests
that the stack overflows in a very particular way. It doesn't seem to test
anything important, and only used to work because we didn't inline into
try-blocks.

BUG=
R=jarin
==========

[bgeron, 2016-08-08 09:30:06]

https://codereview.chromium.org/2216353002/diff/1/src/compiler/js-inlining.cc
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/2216353002/diff/1/src/compiler/js-inlining.cc...
src/compiler/js-inlining.cc:146: const Operator* op = common()->IfException();
On 2016/08/05 14:04:48, Jarin wrote:
> Nit: no need for separate variable, just inline into the NewNode call below.

Done.

https://codereview.chromium.org/2216353002/diff/1/src/compiler/js-inlining.cc...
src/compiler/js-inlining.cc:387: Node* exceptionTarget = nullptr;
On 2016/08/05 14:04:48, Jarin wrote:
> exceptionTarget -> exception_target

Done.

https://codereview.chromium.org/2216353002/diff/1/src/compiler/js-inlining.cc...
src/compiler/js-inlining.cc:476: subcall->opcode() == IrOpcode::kJSCallRuntime)
{
On 2016/08/05 14:04:48, Jarin wrote:
> As discussed offline, you need a more general way:
> 
> I suggest looking for nodes that do not have NoThrow property, and then taking
> the ones that have an IfSuccess use, but no IfException use.

Done.

https://codereview.chromium.org/2216353002/diff/1/src/compiler/js-inlining.cc...
src/compiler/js-inlining.cc:482: s << "found exception node #" << use->id();
On 2016/08/05 14:04:48, Jarin wrote:
> Remove the logging.

Done.

https://codereview.chromium.org/2216353002/diff/1/src/compiler/js-inlining.h
File src/compiler/js-inlining.h (right):

https://codereview.chromium.org/2216353002/diff/1/src/compiler/js-inlining.h#...
src/compiler/js-inlining.h:57: Node* exceptionTarget, NodeVector
uncaught_subcalls);
On 2016/08/05 14:04:48, Jarin wrote:
> exceptionTarget -> exception_target

Done.

[Jarin, 2016-08-10 12:02:52]

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlinin...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:78: NodeVector uncaught_subcalls) {
You should pass the uncought_subcalls as a pointer or (better) as a const
reference. Passing by value will make an unnecessary copy.

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:138: if (uncaught_subcalls.size() > 0) {
Nit: why don't you use subcall_count that you defined just above?

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:482: if (use->opcode() == IrOpcode::kIfSuccess) {
I assume this won't be needed once every no-no-throw node has an ifsuccess
continuation. Right?

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlining.h
File src/compiler/js-inlining.h (right):

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlinin...
src/compiler/js-inlining.h:57: Node* exceptionTarget, NodeVector
uncaught_subcalls);
exceptionTarget -> exception_target

https://codereview.chromium.org/2216353002/diff/80001/test/webkit/fast/js/sta...
File test/webkit/fast/js/stack-overflow-arrity-catch.js (left):

https://codereview.chromium.org/2216353002/diff/80001/test/webkit/fast/js/sta...
test/webkit/fast/js/stack-overflow-arrity-catch.js:1: // Copyright 2013 the V8
project authors. All rights reserved.
Could not we keep the test and instead just change the expectations?

https://codereview.chromium.org/2216353002/diff/80001/tools/gen-inlining-test...
File tools/gen-inlining-tests.py (right):

https://codereview.chromium.org/2216353002/diff/80001/tools/gen-inlining-test...
tools/gen-inlining-tests.py:3: # Copyright 2016 the V8 project authors. All
rights reserved.
Please replace with the short version of the copyright (here and below).

https://codereview.chromium.org/2216353002/diff/80001/tools/gen-inlining-test...
tools/gen-inlining-tests.py:126: os.execl("/usr/bin/env", "env", "python3",
__file__)
Would it be too hard to make this work with python 2. This restarting business
is a bit unfriendly to Windows people or anyone who does not have python in the
standard location.

https://codereview.chromium.org/2216353002/diff/80001/tools/gen-inlining-test...
tools/gen-inlining-tests.py:163: ) = flags
We would also like to test deoptimization. However, that can be added later, and
should not block this CL.

[bgeron, 2016-08-10 14:37:27]

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlinin...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:78: NodeVector uncaught_subcalls) {
On 2016/08/10 12:02:52, Jarin wrote:
> You should pass the uncought_subcalls as a pointer or (better) as a const
> reference. Passing by value will make an unnecessary copy.

Agree, done.

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:138: if (uncaught_subcalls.size() > 0) {
On 2016/08/10 12:02:52, Jarin wrote:
> Nit: why don't you use subcall_count that you defined just above?

Done.

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:482: if (use->opcode() == IrOpcode::kIfSuccess) {
On 2016/08/10 12:02:52, Jarin wrote:
> I assume this won't be needed once every no-no-throw node has an ifsuccess
> continuation. Right?

Hm, I fixed some things to make them saner, but currently some no-no-throw nodes
still lack an IfSuccess continuation, specifically the Throw runtime call (or
something like that) has only an IfException. I'm verifying that the all control
uses of no-no-throw nodes are either IfSuccess or IfException. 

Speaking of which, I just realize that we don't check that no-no-throw nodes
which are reachable from End have any control uses at all. Should we check that?

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlining.h
File src/compiler/js-inlining.h (right):

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlinin...
src/compiler/js-inlining.h:57: Node* exceptionTarget, NodeVector
uncaught_subcalls);
On 2016/08/10 12:02:52, Jarin wrote:
> exceptionTarget -> exception_target

Actually done this time.

https://codereview.chromium.org/2216353002/diff/80001/test/webkit/fast/js/sta...
File test/webkit/fast/js/stack-overflow-arrity-catch.js (left):

https://codereview.chromium.org/2216353002/diff/80001/test/webkit/fast/js/sta...
test/webkit/fast/js/stack-overflow-arrity-catch.js:1: // Copyright 2013 the V8
project authors. All rights reserved.
On 2016/08/10 12:02:52, Jarin wrote:
> Could not we keep the test and instead just change the expectations?

Done; split into two.

https://codereview.chromium.org/2216353002/diff/80001/tools/gen-inlining-test...
File tools/gen-inlining-tests.py (right):

https://codereview.chromium.org/2216353002/diff/80001/tools/gen-inlining-test...
tools/gen-inlining-tests.py:3: # Copyright 2016 the V8 project authors. All
rights reserved.
On 2016/08/10 12:02:52, Jarin wrote:
> Please replace with the short version of the copyright (here and below).

Done.

https://codereview.chromium.org/2216353002/diff/80001/tools/gen-inlining-test...
tools/gen-inlining-tests.py:126: os.execl("/usr/bin/env", "env", "python3",
__file__)
On 2016/08/10 12:02:52, Jarin wrote:
> Would it be too hard to make this work with python 2. This restarting business
> is a bit unfriendly to Windows people or anyone who does not have python in
the
> standard location.

Easier than expected! Done.

https://codereview.chromium.org/2216353002/diff/80001/tools/gen-inlining-test...
tools/gen-inlining-tests.py:163: ) = flags
On 2016/08/10 12:02:52, Jarin wrote:
> We would also like to test deoptimization. However, that can be added later,
and
> should not block this CL.

Ack. Let's look at this together when you're back.

[Jarin, 2016-08-11 09:54:23]

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlinin...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:482: if (use->opcode() == IrOpcode::kIfSuccess) {
On 2016/08/10 14:37:26, bgeron wrote:
> On 2016/08/10 12:02:52, Jarin wrote:
> > I assume this won't be needed once every no-no-throw node has an ifsuccess
> > continuation. Right?
> 
> Hm, I fixed some things to make them saner, but currently some no-no-throw
nodes
> still lack an IfSuccess continuation, specifically the Throw runtime call (or
> something like that) has only an IfException. 

Actually, if Throw does not have IfSuccess, this condition would not be right if
you inline a throw into try-catch.

I would be more comfortable if we did not have to care about presence of
IfSuccess here.

[bgeron, 2016-08-11 18:44:42]

Patchset #7 (id:120001) has been deleted

[bgeron, 2016-08-11 18:55:55]

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlinin...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/2216353002/diff/80001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:482: if (use->opcode() == IrOpcode::kIfSuccess) {
On 2016/08/11 09:54:23, Jarin wrote:
> On 2016/08/10 14:37:26, bgeron wrote:
> > On 2016/08/10 12:02:52, Jarin wrote:
> > > I assume this won't be needed once every no-no-throw node has an ifsuccess
> > > continuation. Right?
> > 
> > Hm, I fixed some things to make them saner, but currently some no-no-throw
> nodes
> > still lack an IfSuccess continuation, specifically the Throw runtime call
(or
> > something like that) has only an IfException. 
> 
> Actually, if Throw does not have IfSuccess, this condition would not be right
if
> you inline a throw into try-catch.
> 
> I would be more comfortable if we did not have to care about presence of
> IfSuccess here.

Removed that condition. 

To make this work, I had to depend on an earlier-landed patch which adds
kNoThrow to some opcodes. This screws up the delta between patch sets, should
you happen to use that.  

Also, I had to disable one assertion in a test: apparently suddenly inlining
works slightly differently now. I don't completely understand how, and I'm not
sure if I care enough.

[Jarin, 2016-08-17 11:20:55]

https://codereview.chromium.org/2216353002/diff/160001/src/compiler/js-inlini...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/2216353002/diff/160001/src/compiler/js-inlini...
src/compiler/js-inlining.cc:482: hasIfException = true;
break; ?

[bgeron, 2016-08-18 17:57:25]

Put inlining into try behind flag. Piggybacking spelling fixes.

Depending on updated tests, which already fail on master for unknown reason.

https://codereview.chromium.org/2216353002/diff/160001/src/compiler/js-inlini...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/2216353002/diff/160001/src/compiler/js-inlini...
src/compiler/js-inlining.cc:482: hasIfException = true;
On 2016/08/17 11:20:55, Jarin wrote:
> break; ?

Done.

[bgeron, 2016-08-25 14:24:18]

The CQ bit was checked by bgeron@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 14:24:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 14:31:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 14:31:18]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/7681)

[bgeron, 2016-08-25 14:56:05]

The CQ bit was checked by bgeron@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 14:56:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 14:58:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 14:58:24]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)

[bgeron, 2016-08-25 15:04:45]

The CQ bit was checked by bgeron@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 15:04:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 15:18:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 15:18:42]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[bgeron, 2016-08-25 15:37:19]

The CQ bit was checked by bgeron@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 15:37:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 15:57:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 15:57:24]

Dry run: This issue passed the CQ dry run.

[bgeron, 2016-08-25 16:27:12]

What do you think Jaro?

[Jarin, 2016-08-29 09:45:00]

lgtm

[bgeron, 2016-08-29 10:52:02]

The CQ bit was checked by bgeron@google.com

[commit-bot: I haz the power, 2016-08-29 10:52:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 11:26:50]

Description was changed from

==========
[turbofan] Also inline into try blocks.

This removes test/webkit/fast/js/stack-overflow-arrity-catch.js, which tests
that the stack overflows in a very particular way. It doesn't seem to test
anything important, and only used to work because we didn't inline into
try-blocks.

BUG=
R=jarin
==========

to

==========
[turbofan] Also inline into try blocks.

This removes test/webkit/fast/js/stack-overflow-arrity-catch.js, which tests
that the stack overflows in a very particular way. It doesn't seem to test
anything important, and only used to work because we didn't inline into
try-blocks.

BUG=
R=jarin
==========

[commit-bot: I haz the power, 2016-08-29 11:26:51]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2016-08-29 11:27:15]

Description was changed from

==========
[turbofan] Also inline into try blocks.

This removes test/webkit/fast/js/stack-overflow-arrity-catch.js, which tests
that the stack overflows in a very particular way. It doesn't seem to test
anything important, and only used to work because we didn't inline into
try-blocks.

BUG=
R=jarin
==========

to

==========
[turbofan] Also inline into try blocks.

This removes test/webkit/fast/js/stack-overflow-arrity-catch.js, which tests
that the stack overflows in a very particular way. It doesn't seem to test
anything important, and only used to work because we didn't inline into
try-blocks.

BUG=
R=jarin

Committed: https://crrev.com/791118fca523d4d868860be68f0a3d520a3a24cd
Cr-Commit-Position: refs/heads/master@{#38976}
==========

[commit-bot: I haz the power, 2016-08-29 11:27:16]

Patchset 13 (id:??) landed as
https://crrev.com/791118fca523d4d868860be68f0a3d520a3a24cd
Cr-Commit-Position: refs/heads/master@{#38976}