[interpreter] Create custom call opcodes for specific argument counts

[interpreter] Create custom call opcodes for specific argument counts

Specifically, add bytecodes for Call0, Call1, Call2, CallProperty0, CallProperty1,
and CallProperty2. Also share the bytecode handler code between between 
equivalent CallX and CallPropertyX handlers.

Review-Url: https://codereview.chromium.org/2684993002
Cr-Original-Commit-Position: refs/heads/master@{#43290}
Committed: https://chromium.googlesource.com/v8/v8/+/00d6f1f80a00c4ac398af588dbd6815395791015
Review-Url: https://codereview.chromium.org/2684993002
Cr-Commit-Position: refs/heads/master@{#43700}
Committed: https://chromium.googlesource.com/v8/v8/+/e466744da5a51fefbec5357a35a17b8fffe987b2

[danno, 2017-02-08 15:44:05]

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

[commit-bot: I haz the power, 2017-02-08 15:44:14]

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, 2017-02-08 15:45:23]

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

[commit-bot: I haz the power, 2017-02-08 15:45:24]

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

[danno, 2017-02-08 17:04:22]

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

[commit-bot: I haz the power, 2017-02-08 17:04: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, 2017-02-08 17:45:05]

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

[commit-bot: I haz the power, 2017-02-08 17:45:06]

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

[danno, 2017-02-09 17:25:50]

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

[commit-bot: I haz the power, 2017-02-09 17:25:56]

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, 2017-02-09 17:53:43]

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

[commit-bot: I haz the power, 2017-02-09 17:53:44]

Dry run: This issue passed the CQ dry run.

[danno, 2017-02-13 17:16:20]

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

[commit-bot: I haz the power, 2017-02-13 17:16:28]

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, 2017-02-13 17:19:35]

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

[commit-bot: I haz the power, 2017-02-13 17:19:36]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/20906)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)

[danno, 2017-02-13 18:07:02]

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

[commit-bot: I haz the power, 2017-02-13 18:07:09]

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, 2017-02-13 18:10:59]

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

[commit-bot: I haz the power, 2017-02-13 18:11:00]

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

[danno, 2017-02-13 21:19:48]

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

[commit-bot: I haz the power, 2017-02-13 21:19:51]

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, 2017-02-13 21:27:02]

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

[commit-bot: I haz the power, 2017-02-13 21:27:03]

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

[danno, 2017-02-13 21:48:35]

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

[commit-bot: I haz the power, 2017-02-13 21:48:38]

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, 2017-02-13 22:23:12]

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

[commit-bot: I haz the power, 2017-02-13 22:23:14]

Dry run: This issue passed the CQ dry run.

[danno, 2017-02-16 22:54:29]

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

[commit-bot: I haz the power, 2017-02-16 22:54:35]

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, 2017-02-16 23:20:54]

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

[commit-bot: I haz the power, 2017-02-16 23:20:55]

Dry run: This issue passed the CQ dry run.

[danno, 2017-02-17 07:39:57]

danno@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[danno, 2017-02-17 07:39:59]

ptal

[rmcilroy, 2017-02-17 11:37:20]

LGTM once comments are addressed, thanks!

https://codereview.chromium.org/2684993002/diff/200001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2684993002/diff/200001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:1328: void
BytecodeGraphBuilder::BuildCall(TailCallMode tail_call_mode,
nit - BuildCallN or something to specify this is for building a call from a
bytecode with a variable arg count? 

Edit: From looking at other places it seems you use CallN to specify the
bytecodes with known number of arguments, so BuildCallN doesn't work here, but
I'd still prefer if there was some distinction in names if you can think of
something.

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:467: return
SmiTag(BytecodeOperandIdx(operand_index));
nit - could this use the new BytecodeOperandIdxInt32, or does SmiTag require a
word representation input?

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

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.cc:154: static_cast<int>(Bytecode::kCall2));
Can you also DCHECK that offset < index

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.cc:157: }
Could we pull the duplication logic out to a separate helper function and run
the check at the top, e.g.:

if (!Bytecodes::BytecodeHasHandler(bytecode, operand_scale)) return;
if (ReuseExistingHandler(bytecode, operand_scale)) return;

where ReuseExistingHandler updates the dispatch table.

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.cc:2181: void
Interpreter::DoJSCallN(InterpreterAssembler* assembler, int n) {
nit - n -> arg_count

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.cc:2185: const int kReceiveParameterOffset = 5;
nit - receiverParameterOffset

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.cc:2190: for (int i = 0; i < (n +
kReceiverOperandCount); ++i) {
nit - could you do these in-order (the for loop between temp[4] and temp[n+6],

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.cc:2191: Node* reg = __ BytecodeOperandReg(i +
kFunctionOperandCount);
nit - I find the use of kFunctionOperandCount and kReceiverOperandCount a bit
confusing. How about:
  kRecieverOperandIndex = 1;
  kRecieverCount = 1;
  kSlotOperandIndex = kRecieverOperandIndex + kRecieverOperandCount + n;

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.cc:2201: temp[n + 6] = __ GetContext();
nit - temp[5 + kRecieverCount + n]

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.h:127: // Generates code to perform a JS call that
collects type feedback.
nit - JS call with a known number of arguments...

[danno, 2017-02-17 17:17:02]

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

[commit-bot: I haz the power, 2017-02-17 17:17:11]

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

[danno, 2017-02-17 17:18:23]

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

[commit-bot: I haz the power, 2017-02-17 17:18:31]

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

[danno, 2017-02-17 17:18:43]

Feedback addressed will land when bots go green.

https://codereview.chromium.org/2684993002/diff/200001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2684993002/diff/200001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:1328: void
BytecodeGraphBuilder::BuildCall(TailCallMode tail_call_mode,
On 2017/02/17 11:37:19, rmcilroy wrote:
> nit - BuildCallN or something to specify this is for building a call from a
> bytecode with a variable arg count? 
> 
> Edit: From looking at other places it seems you use CallN to specify the
> bytecodes with known number of arguments, so BuildCallN doesn't work here, but
> I'd still prefer if there was some distinction in names if you can think of
> something.

Done.

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:467: return
SmiTag(BytecodeOperandIdx(operand_index));
On 2017/02/17 11:37:19, rmcilroy wrote:
> nit - could this use the new BytecodeOperandIdxInt32, or does SmiTag require a
> word representation input?

It requires a word. Otherwise, it needs to be explicitly sign-extended.

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

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.cc:154: static_cast<int>(Bytecode::kCall2));
On 2017/02/17 11:37:19, rmcilroy wrote:
> Can you also DCHECK that offset < index

Done.

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.cc:157: }
On 2017/02/17 11:37:19, rmcilroy wrote:
> Could we pull the duplication logic out to a separate helper function and run
> the check at the top, e.g.:
> 
> if (!Bytecodes::BytecodeHasHandler(bytecode, operand_scale)) return;
> if (ReuseExistingHandler(bytecode, operand_scale)) return;
> 
> where ReuseExistingHandler updates the dispatch table.

Done.

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.cc:2181: void
Interpreter::DoJSCallN(InterpreterAssembler* assembler, int n) {
On 2017/02/17 11:37:19, rmcilroy wrote:
> nit - n -> arg_count

Done.

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.cc:2185: const int kReceiveParameterOffset = 5;
On 2017/02/17 11:37:19, rmcilroy wrote:
> nit - receiverParameterOffset

Done.

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.cc:2190: for (int i = 0; i < (n +
kReceiverOperandCount); ++i) {
On 2017/02/17 11:37:19, rmcilroy wrote:
> nit - could you do these in-order (the for loop between temp[4] and temp[n+6],


Done.

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.cc:2191: Node* reg = __ BytecodeOperandReg(i +
kFunctionOperandCount);
On 2017/02/17 11:37:19, rmcilroy wrote:
> nit - I find the use of kFunctionOperandCount and kReceiverOperandCount a bit
> confusing. How about:
>   kRecieverOperandIndex = 1;
>   kRecieverCount = 1;
>   kSlotOperandIndex = kRecieverOperandIndex + kRecieverOperandCount + n;  

Done.

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.cc:2201: temp[n + 6] = __ GetContext();
On 2017/02/17 11:37:19, rmcilroy wrote:
> nit - temp[5 + kRecieverCount + n]

Done.

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/2684993002/diff/200001/src/interpreter/interp...
src/interpreter/interpreter.h:127: // Generates code to perform a JS call that
collects type feedback.
On 2017/02/17 11:37:19, rmcilroy wrote:
> nit - JS call with a known number of arguments...

Done.

[danno, 2017-02-17 17:54:38]

Description was changed from

==========
[interpreter] Create custom call opcodes for specific argument counts
==========

to

==========
[interpreter] Create custom call opcodes for specific argument counts

Specifically, add bytecodes for Call0, Call1, Call2, CallProperty0,
CallProperty1,
and CallProperty2. Also share the bytecode handler code between between 
equivalent CallX and CallPropertyX handlers.
==========

[commit-bot: I haz the power, 2017-02-17 17:54:48]

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

[commit-bot: I haz the power, 2017-02-17 17:54:49]

Dry run: This issue passed the CQ dry run.

[danno, 2017-02-17 17:55:29]

The CQ bit was checked by danno@chromium.org

[danno, 2017-02-17 17:55:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2684993002/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-17 17:55:37]

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, 2017-02-17 17:57:23]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1487354129853580,
"parent_rev": "3e1db847b36885f2c40362d88884b7ef65af8271", "commit_rev":
"00d6f1f80a00c4ac398af588dbd6815395791015"}

[commit-bot: I haz the power, 2017-02-17 17:57:28]

Description was changed from

==========
[interpreter] Create custom call opcodes for specific argument counts

Specifically, add bytecodes for Call0, Call1, Call2, CallProperty0,
CallProperty1,
and CallProperty2. Also share the bytecode handler code between between 
equivalent CallX and CallPropertyX handlers.
==========

to

==========
[interpreter] Create custom call opcodes for specific argument counts

Specifically, add bytecodes for Call0, Call1, Call2, CallProperty0,
CallProperty1,
and CallProperty2. Also share the bytecode handler code between between 
equivalent CallX and CallPropertyX handlers.

Review-Url: https://codereview.chromium.org/2684993002
Cr-Commit-Position: refs/heads/master@{#43290}
Committed:
https://chromium.googlesource.com/v8/v8/+/00d6f1f80a00c4ac398af588dbd68153957...
==========

[commit-bot: I haz the power, 2017-02-17 17:57:30]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/v8/v8/+/00d6f1f80a00c4ac398af588dbd68153957...

[Michael Achenbach, 2017-02-20 07:43:03]

On 2017/02/17 17:57:30, commit-bot: I haz the power wrote:
> Committed patchset #13 (id:240001) as
>
https://chromium.googlesource.com/v8/v8/+/00d6f1f80a00c4ac398af588dbd68153957...

This seems to cause a dcheck failure in arm64 nosnap debug:
https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%...

[danno, 2017-02-20 09:24:08]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/2709533002/ by danno@chromium.org.

The reason for reverting is: Due to arm64 failures.

[rmcilroy, 2017-02-20 10:11:33]

On 2017/02/20 09:24:08, danno wrote:
> A revert of this CL (patchset #13 id:240001) has been created in
> https://codereview.chromium.org/2709533002/ by mailto:danno@chromium.org.
> 
> The reason for reverting is: Due to arm64 failures.

It also didn't seem to make any difference in (positive or negative) performance
unfortunately (which totally surprises me):

https://chromeperf.appspot.com/report?sid=e2680c7a78c63d50c318d01f0472f0788f1...

[danno, 2017-03-09 14:00:50]

Description was changed from

==========
[interpreter] Create custom call opcodes for specific argument counts

Specifically, add bytecodes for Call0, Call1, Call2, CallProperty0,
CallProperty1,
and CallProperty2. Also share the bytecode handler code between between 
equivalent CallX and CallPropertyX handlers.

Review-Url: https://codereview.chromium.org/2684993002
Cr-Commit-Position: refs/heads/master@{#43290}
Committed:
https://chromium.googlesource.com/v8/v8/+/00d6f1f80a00c4ac398af588dbd68153957...
==========

to

==========
[interpreter] Create custom call opcodes for specific argument counts

Specifically, add bytecodes for Call0, Call1, Call2, CallProperty0,
CallProperty1,
and CallProperty2. Also share the bytecode handler code between between 
equivalent CallX and CallPropertyX handlers.

Review-Url: https://codereview.chromium.org/2684993002
Cr-Commit-Position: refs/heads/master@{#43290}
Committed:
https://chromium.googlesource.com/v8/v8/+/00d6f1f80a00c4ac398af588dbd68153957...
==========

[danno, 2017-03-09 14:00:57]

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

[commit-bot: I haz the power, 2017-03-09 14:01:06]

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, 2017-03-09 14:30:00]

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

[commit-bot: I haz the power, 2017-03-09 14:30:02]

Dry run: This issue passed the CQ dry run.

[danno, 2017-03-09 14:38:19]

The CQ bit was checked by danno@chromium.org

[danno, 2017-03-09 14:38:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2684993002/#ps260001
(title: "Fix golden files again")

[commit-bot: I haz the power, 2017-03-09 14:38:23]

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, 2017-03-09 14:40:03]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1489070299879910,
"parent_rev": "18be5d7057d794dbe3b663dfc17d3e512f2b55e3", "commit_rev":
"e466744da5a51fefbec5357a35a17b8fffe987b2"}

[commit-bot: I haz the power, 2017-03-09 14:40:08]

Description was changed from

==========
[interpreter] Create custom call opcodes for specific argument counts

Specifically, add bytecodes for Call0, Call1, Call2, CallProperty0,
CallProperty1,
and CallProperty2. Also share the bytecode handler code between between 
equivalent CallX and CallPropertyX handlers.

Review-Url: https://codereview.chromium.org/2684993002
Cr-Commit-Position: refs/heads/master@{#43290}
Committed:
https://chromium.googlesource.com/v8/v8/+/00d6f1f80a00c4ac398af588dbd68153957...
==========

to

==========
[interpreter] Create custom call opcodes for specific argument counts

Specifically, add bytecodes for Call0, Call1, Call2, CallProperty0,
CallProperty1,
and CallProperty2. Also share the bytecode handler code between between 
equivalent CallX and CallPropertyX handlers.

Review-Url: https://codereview.chromium.org/2684993002
Cr-Original-Commit-Position: refs/heads/master@{#43290}
Committed:
https://chromium.googlesource.com/v8/v8/+/00d6f1f80a00c4ac398af588dbd68153957...
Review-Url: https://codereview.chromium.org/2684993002
Cr-Commit-Position: refs/heads/master@{#43700}
Committed:
https://chromium.googlesource.com/v8/v8/+/e466744da5a51fefbec5357a35a17b8fffe...
==========

[commit-bot: I haz the power, 2017-03-09 14:40:09]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/v8/v8/+/e466744da5a51fefbec5357a35a17b8fffe...