[interpreter] Streamline bytecode array writing.

[interpreter] Streamline bytecode array writing.

Simplify bytecode array writing and remove some now unused bytecode traits
definitions.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/3bc6cc4feeddbae321b2b42b9237fb9be62041de
Cr-Commit-Position: refs/heads/master@{#37321}

[oth, 2016-06-27 13:26:33]

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

[oth, 2016-06-27 13:26:33]

Ross, PTAL. There are some mildly positive deltas in the perf try jobs for
octane-noopt. Thanks.

[rmcilroy, 2016-06-27 14:03:48]

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-writer.cc (right):

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-writer.cc:210: for (int i = 0; i < operand_count;
++i) {
Could we keep doing this as part of the loop above? Any reason for the change of
approach here compared to the previous bitmap approach?

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-writer.cc:216: if (operand_types[i + 1] ==
OperandType::kRegCount) {
I've been caught out again thinking this goes out-of-bounds. Could you add a
comment that the list is terminated by kNone so the size of the list is actually
one larger than the operand count (I find this pretty confusing TBH, but can
live with it for now).

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-writer.cc:220: max_register_count_ =
std::max(max_register_count_, reg.index() + count);
Any idea how much performance this part of the loop costs us? It seems like
something the register allocator should keep track of (with input from REO)
rather than us having to track each operand at this stage. Just a thought, not
blocking this CL.

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-writer.h (right):

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-writer.h:41: 2 * sizeof(Bytecode) +
Bytecodes::kMaxOperands * sizeof(uint32_t);
/s/uint32_t/OperandSize::kLast ?

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-re...
File src/interpreter/bytecode-register-optimizer.cc (right):

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-re...
src/interpreter/bytecode-register-optimizer.cc:513: if (operand_types[i + 1] ==
OperandType::kRegCount) {
Ditto

[oth, 2016-06-27 18:52:35]

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

[commit-bot: I haz the power, 2016-06-27 18:52:39]

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

[oth, 2016-06-27 18:53:50]

Thanks, comments incorporated. PTAL.

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-writer.cc (right):

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-writer.cc:210: for (int i = 0; i < operand_count;
++i) {
On 2016/06/27 14:03:48, rmcilroy wrote:
> Could we keep doing this as part of the loop above? Any reason for the change
of
> approach here compared to the previous bitmap approach?

Done.

It makes no difference either way perf wise. As you look at the code, it makes
two distinct blocks (candidate functions) for packing data and doing the maximum
register computation. No attachment to this change.

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-writer.cc:216: if (operand_types[i + 1] ==
OperandType::kRegCount) {
On 2016/06/27 14:03:48, rmcilroy wrote:
> I've been caught out again thinking this goes out-of-bounds. Could you add a
> comment that the list is terminated by kNone so the size of the list is
actually
> one larger than the operand count (I find this pretty confusing TBH, but can
> live with it for now).

Done. It's no more or less confusing than having a terminator character in
C-strings. It's documented in bytecodes.h and now everywhere it's used. Fine
here if you want to remove it, but that's not something I am interested in doing
in this CL.

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-writer.cc:220: max_register_count_ =
std::max(max_register_count_, reg.index() + count);
On 2016/06/27 14:03:48, rmcilroy wrote:
> Any idea how much performance this part of the loop costs us? It seems like
> something the register allocator should keep track of (with input from REO)
> rather than us having to track each operand at this stage. Just a thought, not
> blocking this CL.

Three runs on my workstation suggest it is in the noise.

Not having this calculation: 1889, 2033, 1988.
Having this calculation: 1985, 2013, 1976.

I suspect it's cleaner to do it in the final stage than add plumbing between REO
and here to calculate this. It's also conceivable that another stage could
remove temporary registers (LDAR/STAR elimination in peephole optimizer could in
principle). The ground truth is what reaches the final stage.

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-writer.h (right):

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-writer.h:41: 2 * sizeof(Bytecode) +
Bytecodes::kMaxOperands * sizeof(uint32_t);
On 2016/06/27 14:03:48, rmcilroy wrote:
> /s/uint32_t/OperandSize::kLast ?

Done.

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-re...
File src/interpreter/bytecode-register-optimizer.cc (right):

https://codereview.chromium.org/2100793003/diff/1/src/interpreter/bytecode-re...
src/interpreter/bytecode-register-optimizer.cc:513: if (operand_types[i + 1] ==
OperandType::kRegCount) {
On 2016/06/27 14:03:48, rmcilroy wrote:
> Ditto

Done.

[commit-bot: I haz the power, 2016-06-27 19:16:57]

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

[commit-bot: I haz the power, 2016-06-27 19:16:58]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-06-28 09:07:26]

lgtm

https://codereview.chromium.org/2100793003/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-writer.h (right):

https://codereview.chromium.org/2100793003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-writer.h:42: Bytecodes::kMaxOperands *
sizeof(static_cast<size_t>(OperandSize::kLast));
I don't think you want the sizeof here any longer, OperandSize::kLast gives you
the size directly

[oth, 2016-06-28 09:25:32]

On 2016/06/28 09:07:26, rmcilroy wrote:
> lgtm
> 
>
https://codereview.chromium.org/2100793003/diff/40001/src/interpreter/bytecod...
> File src/interpreter/bytecode-array-writer.h (right):
> 
>
https://codereview.chromium.org/2100793003/diff/40001/src/interpreter/bytecod...
> src/interpreter/bytecode-array-writer.h:42: Bytecodes::kMaxOperands *
> sizeof(static_cast<size_t>(OperandSize::kLast));
> I don't think you want the sizeof here any longer, OperandSize::kLast gives
you
> the size directly

Oops, good spot. Thanks!

[oth, 2016-06-28 09:34:08]

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

[commit-bot: I haz the power, 2016-06-28 09:34:18]

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-06-28 09:58:49]

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

[commit-bot: I haz the power, 2016-06-28 09:58:50]

Dry run: This issue passed the CQ dry run.

[oth, 2016-06-28 10:02:12]

The CQ bit was checked by oth@chromium.org

[oth, 2016-06-28 10:02:12]

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/2100793003/#ps80001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-06-28 10:02:19]

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-06-28 10:04:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-28 10:05:01]

Description was changed from

==========
[interpreter] Streamline bytecode array writing.

Simplify bytecode array writing and remove some now unused bytecode traits
definitions.

BUG=v8:4280
LOG=N
==========

to

==========
[interpreter] Streamline bytecode array writing.

Simplify bytecode array writing and remove some now unused bytecode traits
definitions.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/3bc6cc4feeddbae321b2b42b9237fb9be62041de
Cr-Commit-Position: refs/heads/master@{#37321}
==========

[commit-bot: I haz the power, 2016-06-28 10:05:02]

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