[turbofan] JSGenericLowering mostly uses builtins instead of code stubs now

[turbofan] JSGenericLowering mostly uses builtins instead of code stubs now

BUG=v8:5431
Committed: https://crrev.com/0c168a90ffc34e3e4defee0c1cc39fe522dae2c0
Cr-Commit-Position: refs/heads/master@{#40051}

[Tobias Tebbi, 2016-09-27 15:04:56]

Description was changed from

==========
[turbofan] JSGenericLowering uses builtins instead of code stubs now

BUG=
==========

to

==========
[turbofan] JSGenericLowering uses builtins instead of code stubs now

BUG=v8:5431
==========

[Tobias Tebbi, 2016-09-28 09:18:02]

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

[Tobias Tebbi, 2016-09-28 09:22:30]

This is just a draft, I just want to know if my approach is OK.

[Tobias Tebbi, 2016-10-04 12:51:41]

Patchset #4 (id:60001) has been deleted

[Tobias Tebbi, 2016-10-04 12:51:51]

Patchset #3 (id:40001) has been deleted

[Benedikt Meurer, 2016-10-05 05:41:30]

bmeurer@chromium.org changed reviewers:
+ ishell@chromium.org, rmcilroy@chromium.org

[Benedikt Meurer, 2016-10-05 05:41:30]

bmeurer@chromium.org changed required reviewers:
+ ishell@chromium.org, rmcilroy@chromium.org

[Benedikt Meurer, 2016-10-05 05:41:30]

LGTM from my side. Adding Ross for interpreter and Igor for CodeStubAssembler.

https://codereview.chromium.org/2372113004/diff/80001/src/builtins/builtins.cc
File src/builtins/builtins.cc (right):

https://codereview.chromium.org/2372113004/diff/80001/src/builtins/builtins.c...
src/builtins/builtins.cc:8: #include "src/code-stubs.h"
This seems to be unnecessary now.

https://codereview.chromium.org/2372113004/diff/80001/src/builtins/builtins.c...
src/builtins/builtins.cc:113: 
Same here.

[Tobias Tebbi, 2016-10-05 06:04:29]

There is still a bug in the CL: it crashes when generating the snapshot. I am
investigating it.

[Tobias Tebbi, 2016-10-05 09:53:14]

The CQ bit was checked by tebbi@chromium.org to run a CQ dry run

[Tobias Tebbi, 2016-10-05 09:53:16]

The CQ bit was unchecked by tebbi@chromium.org

[Tobias Tebbi, 2016-10-05 14:36:18]

Description was changed from

==========
[turbofan] JSGenericLowering uses builtins instead of code stubs now

BUG=v8:5431
==========

to

==========
[turbofan] JSGenericLowering mostly uses builtins instead of code stubs now

BUG=v8:5431
==========

[Tobias Tebbi, 2016-10-05 14:36:36]

It works now, but is still incomplete: there are still Stubs being loaded in
JSGenericLowering. However, I would like this to be landed nevertheless, because
the moved code creates painful manual merging.

[Tobias Tebbi, 2016-10-05 15:57:34]

The CQ bit was checked by tebbi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 15:57:42]

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-10-05 16:48:22]

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

[commit-bot: I haz the power, 2016-10-05 16:48:23]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-10-05 16:53:14]

Yes, I think this is landable as-is and make sense.

Ping Igor / Ross.

[Igor Sheludko, 2016-10-05 17:05:06]

On 2016/10/05 16:53:14, Benedikt Meurer wrote:
> Yes, I think this is landable as-is and make sense.
> 
> Ping Igor / Ross.

Reviewing. Will finish tomorrow.

[rmcilroy, 2016-10-05 17:20:52]

https://codereview.chromium.org/2372113004/diff/180001/src/interpreter/interp...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2372113004/diff/180001/src/interpreter/interp...
src/interpreter/interpreter.cc:1711: context);
I'm not keen on this approach of building a generator that looks like the old
code-stub Generate function. How about just passing the appropriate AST Token
into DoCompareOpWithFeedback, and switch on the token to choose the right
assembler operator (similar to DoBitwiseBinaryOp)?

https://codereview.chromium.org/2372113004/diff/180001/src/interpreter/interp...
src/interpreter/interpreter.cc:1795: DoBinaryOp<Generator>(assembler);
Ditto for DoBinaryOp (maybe these should be calling a function called
DoCompareOp (WithoutFeedback).

[Igor Sheludko, 2016-10-06 08:51:01]

I'm sorry, but I would really prefer such a CL to be cut into 5-7 pieces.
Otherwise it's unreviewable :(
CodeStubAssembler/builtins part rubberstamp-lgtm.

https://codereview.chromium.org/2372113004/diff/180001/src/builtins/builtins-...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2372113004/diff/180001/src/builtins/builtins-...
src/builtins/builtins-string.cc:193: enum RelationalComparisonMode {
We have a CSA::RelationalComparisonMode. Is this one still necessary.

https://codereview.chromium.org/2372113004/diff/180001/src/builtins/builtins-...
src/builtins/builtins-string.cc:200: void
GenerateStringRelationalComparison(CodeStubAssembler* assembler,
I guess this thing is now called CSA::RelationalComparison(). Maybe you should
remove it from here.

[Tobias Tebbi, 2016-10-06 11:01:17]

I addressed the comments, thank you!

https://codereview.chromium.org/2372113004/diff/200001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2372113004/diff/200001/src/code-stub-assemble...
src/code-stub-assembler.h:853: Runtime::FunctionId fallback_runtime_function_id
= Runtime::kHasProperty);
@Igor: I also made this argument optional to hide this implementation detail.

[Tobias Tebbi, 2016-10-06 11:41:39]

The CQ bit was checked by tebbi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 11:41:46]

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-10-06 11:43:14]

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

[commit-bot: I haz the power, 2016-10-06 11:43:15]

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

[Tobias Tebbi, 2016-10-06 11:45:45]

The CQ bit was checked by tebbi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 11:45:54]

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-10-06 12:15:33]

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

[commit-bot: I haz the power, 2016-10-06 12:15:34]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-10-06 15:00:13]

interpreter/ LGTM with two final comments. Thanks.

https://codereview.chromium.org/2372113004/diff/220001/src/interpreter/interp...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2372113004/diff/220001/src/interpreter/interp...
src/interpreter/interpreter.cc:907: // TODO(mythria): Remove this function once
all BinaryOps record type feedback.
Please update this comment (BinaryOps -> CompareOps)

https://codereview.chromium.org/2372113004/diff/220001/src/interpreter/interp...
src/interpreter/interpreter.cc:1570: };
TypeOf is the only non-feedback unary op, please just inline the DoUnaryOp code
into DoTypeOf instead of building the generator type.

[Tobias Tebbi, 2016-10-06 15:15:40]

The CQ bit was checked by tebbi@chromium.org

[Tobias Tebbi, 2016-10-06 15:15:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, ishell@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2372113004/#ps240001
(title: "Ross' comments")

[commit-bot: I haz the power, 2016-10-06 15:15:53]

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-10-06 15:46:14]

Description was changed from

==========
[turbofan] JSGenericLowering mostly uses builtins instead of code stubs now

BUG=v8:5431
==========

to

==========
[turbofan] JSGenericLowering mostly uses builtins instead of code stubs now

BUG=v8:5431
==========

[commit-bot: I haz the power, 2016-10-06 15:46:16]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2016-10-06 15:46:34]

Description was changed from

==========
[turbofan] JSGenericLowering mostly uses builtins instead of code stubs now

BUG=v8:5431
==========

to

==========
[turbofan] JSGenericLowering mostly uses builtins instead of code stubs now

BUG=v8:5431

Committed: https://crrev.com/0c168a90ffc34e3e4defee0c1cc39fe522dae2c0
Cr-Commit-Position: refs/heads/master@{#40051}
==========

[commit-bot: I haz the power, 2016-10-06 15:46:36]

Patchset 11 (id:??) landed as
https://crrev.com/0c168a90ffc34e3e4defee0c1cc39fe522dae2c0
Cr-Commit-Position: refs/heads/master@{#40051}