[interpreter] Remove OperandScale from front stages of pipeline.

[interpreter] Remove OperandScale from front stages of pipeline.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/85882a6320703df3db9bbf8f85c6e82df4c87ef4
Cr-Commit-Position: refs/heads/master@{#36885}

[oth, 2016-06-06 20:40:35]

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

[oth, 2016-06-06 20:40:35]

This seems broadly perf neutral, but cleans up dealing with operand scales from
the bytecode array builder until the bytecode array writer.

There's still some more clean-up work and putting back operand verification for
debug builds.

Any opinion on doing this? Definitely not high priority, but will finish it off
if so.

Cheers
O.

[rmcilroy, 2016-06-07 10:32:47]

Nice, I like it! As you say, this is low-prio, but I think it's a good cleanup
(you could make it a PE-fixit bug ;)).

https://codereview.chromium.org/2041913002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2041913002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:282: static uint32_t
UnsignedOperand(size_t value);
Do we need the differentiation between signed and unsigned here any longer? I
think we could just have ToOperand(int/size_t) and do the checks for correct
size for signed / unsigned in bytecode array builder?

https://codereview.chromium.org/2041913002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-writer.cc (right):

https://codereview.chromium.org/2041913002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-writer.cc:120:
Bytecodes::GetOperandTypes(node->bytecode());
Could we just add Bytecodes::GetOperandTypeInfos instead of translating from
Type to TypeInfo here?

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

https://codereview.chromium.org/2041913002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-writer.h:40: static const uint32_t
k8BitPlaceholder = 0x7f;
kJump8BitPlaceholder (and similar)

https://codereview.chromium.org/2041913002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/2041913002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:599: return SizeForUnsignedOperand(unsigned_value *
2u);
I prefer the old approach we used here. Is there any reason you changed it?

[oth, 2016-06-08 15:08:41]

Thanks, revised and progressed.

https://codereview.chromium.org/2041913002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2041913002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:282: static uint32_t
UnsignedOperand(size_t value);
On 2016/06/07 10:32:46, rmcilroy wrote:
> Do we need the differentiation between signed and unsigned here any longer? I
> think we could just have ToOperand(int/size_t) and do the checks for correct
> size for signed / unsigned in bytecode array builder?

It's doable, but doing the check upfront means it fails immediately rather than
after going through the pipeline. The status quo seems better here, WDYT?

https://codereview.chromium.org/2041913002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-writer.cc (right):

https://codereview.chromium.org/2041913002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-writer.cc:120:
Bytecodes::GetOperandTypes(node->bytecode());
On 2016/06/07 10:32:46, rmcilroy wrote:
> Could we just add Bytecodes::GetOperandTypeInfos instead of translating from
> Type to TypeInfo here? 

Done.

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

https://codereview.chromium.org/2041913002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-writer.h:40: static const uint32_t
k8BitPlaceholder = 0x7f;
On 2016/06/07 10:32:46, rmcilroy wrote:
> kJump8BitPlaceholder (and similar)

Done.

https://codereview.chromium.org/2041913002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/2041913002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:599: return SizeForUnsignedOperand(unsigned_value *
2u);
On 2016/06/07 10:32:46, rmcilroy wrote:
> I prefer the old approach we used here. Is there any reason you changed it?

Done. This was just experimentation with slightly fewer branches.

[rmcilroy, 2016-06-09 15:16:36]

LGTM with comments.

https://codereview.chromium.org/2041913002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-array-writer.cc (right):

https://codereview.chromium.org/2041913002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-array-writer.cc:124: return OperandSize::kByte;
kShort

https://codereview.chromium.org/2041913002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-array-writer.cc:135:
Bytecodes::GetOperandTypeInfos(node->bytecode());
Rather than doing this dance between Size and Scale, how about just checking
scalable operands, and having a OperandScaleFor[Un]SignedOperand and get the
maximum value for all scalable operands?

[oth, 2016-06-09 19:59:08]

Thanks, comments incorporated (though Rietveld is not showing the latest
uploaded CL just now).

https://codereview.chromium.org/2041913002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-array-writer.cc (right):

https://codereview.chromium.org/2041913002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-array-writer.cc:124: return OperandSize::kByte;
On 2016/06/09 15:16:35, rmcilroy wrote:
> kShort

For the daft logic here, it's effectively 1 so Byte is the right enumeration.
This path needs to change.

https://codereview.chromium.org/2041913002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-array-writer.cc:135:
Bytecodes::GetOperandTypeInfos(node->bytecode());
On 2016/06/09 15:16:35, rmcilroy wrote:
> Rather than doing this dance between Size and Scale, how about just checking
> scalable operands, and having a OperandScaleFor[Un]SignedOperand and get the
> maximum value for all scalable operands?

Done.

[oth, 2016-06-10 10:11:50]

The CQ bit was checked by oth@chromium.org

[oth, 2016-06-10 10:11:50]

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/2041913002/#ps220001
(title: "Rebase")

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

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

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

Committed patchset #12 (id:220001)

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

Description was changed from

==========
[interpreter] Remove OperandScale from front stages of pipeline.

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

to

==========
[interpreter] Remove OperandScale from front stages of pipeline.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/85882a6320703df3db9bbf8f85c6e82df4c87ef4
Cr-Commit-Position: refs/heads/master@{#36885}
==========

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

Patchset 12 (id:??) landed as
https://crrev.com/85882a6320703df3db9bbf8f85c6e82df4c87ef4
Cr-Commit-Position: refs/heads/master@{#36885}