[Interpreter] Bytecodes for exchanging registers.

[Interpreter] Bytecodes for exchanging registers.

New bytecodes for making registers with indicies wider than 1-byte
accessible.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/c958c98c4f11e386d4efc25da64b310e139bb62f
Cr-Commit-Position: refs/heads/master@{#33091}

[oth, 2016-01-04 11:12:49]

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

[oth, 2016-01-04 11:12:50]

First of two part change introducing a register window in the bytecode
generation path so most bytecodes only need 8-bit register operands.

rmcilroy@chromium.org: Please review changes all over.

bmeurer@chromium.org: Please review changes in compiler.

[Benedikt Meurer, 2016-01-04 11:19:41]

LGTM on compiler.

[rmcilroy, 2016-01-04 11:54:54]

Looks good overall, a couple of comments.

https://codereview.chromium.org/1555713002/diff/20001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1555713002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:175: interpreter::Register r0,
interpreter::Register r1) {
nit - reg0, reg1

https://codereview.chromium.org/1555713002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:176: int i0 = RegisterToValuesIndex(r0);
nit - reg0_index

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:383: Register b) {
nit - a->lhs  b->rhs (like .h file)

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:384: if (a != b) {
Should we ever call Exchange when the registers are the same? Could this be a
DCHECK instead?

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1189: reg.is_new_target()) {
Do we ever want to allow exchanges of these registers? That would seem to be a
bug (since the interpreter might assume the context / closure / etc. is in these
fixed registers).

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

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:279: os << "r" << reg.index();
nit - add "a" argument case as well ?

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:32: V(Idx16, OperandSize::kShort)    \
nit - alphabetical ordering?

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:82: V(Exchange, OperandType::kReg8,
OperandType::kReg16)                         \
Is it worth having the non-wide version? It only saves a byte, and we should
only hopefully not hit the exchange path too often (otherwise we have more to
worry about, with the extra exchange bytecodes and the extra byte here wouldn't
have much impact). WDYT?

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:256: static const int kMinRegisterIndex = -128;
Can we get rid of these now? If not could we rename them more appropriately?

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:211: Node* v1 = __ LoadRegister(r1);
nit - non-single character variable names.

[oth, 2016-01-04 13:37:48]

On 2016/01/04 11:54:54, rmcilroy wrote:
> Looks good overall, a couple of comments.
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/compiler/bytecode-g...
> File src/compiler/bytecode-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/compiler/bytecode-g...
> src/compiler/bytecode-graph-builder.cc:175: interpreter::Register r0,
> interpreter::Register r1) {
> nit - reg0, reg1
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/compiler/bytecode-g...
> src/compiler/bytecode-graph-builder.cc:176: int i0 =
RegisterToValuesIndex(r0);
> nit - reg0_index
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
> File src/interpreter/bytecode-array-builder.cc (right):
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
> src/interpreter/bytecode-array-builder.cc:383: Register b) {
> nit - a->lhs  b->rhs (like .h file)
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
> src/interpreter/bytecode-array-builder.cc:384: if (a != b) {
> Should we ever call Exchange when the registers are the same? Could this be a
> DCHECK instead?
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
> src/interpreter/bytecode-array-builder.cc:1189: reg.is_new_target()) {
> Do we ever want to allow exchanges of these registers? That would seem to be a
> bug (since the interpreter might assume the context / closure / etc. is in
these
> fixed registers).
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
> File src/interpreter/bytecodes.cc (right):
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
> src/interpreter/bytecodes.cc:279: os << "r" << reg.index();
> nit - add "a" argument case as well ?
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
> File src/interpreter/bytecodes.h (right):
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
> src/interpreter/bytecodes.h:32: V(Idx16, OperandSize::kShort)    \
> nit - alphabetical ordering?
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
> src/interpreter/bytecodes.h:82: V(Exchange, OperandType::kReg8,
> OperandType::kReg16)                         \
> Is it worth having the non-wide version? It only saves a byte, and we should
> only hopefully not hit the exchange path too often (otherwise we have more to
> worry about, with the extra exchange bytecodes and the extra byte here
wouldn't
> have much impact). WDYT?
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
> src/interpreter/bytecodes.h:256: static const int kMinRegisterIndex = -128;
> Can we get rid of these now? If not could we rename them more appropriately?
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/interpr...
> File src/interpreter/interpreter.cc (right):
> 
>
https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/interpr...
> src/interpreter/interpreter.cc:211: Node* v1 = __ LoadRegister(r1);
> nit - non-single character variable names.

Thanks, all addressed.

[oth, 2016-01-04 14:01:42]

Thanks, missed attaching the comments.

https://codereview.chromium.org/1555713002/diff/20001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1555713002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:175: interpreter::Register r0,
interpreter::Register r1) {
On 2016/01/04 11:54:53, rmcilroy wrote:
> nit - reg0, reg1

Done.

https://codereview.chromium.org/1555713002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:176: int i0 = RegisterToValuesIndex(r0);
On 2016/01/04 11:54:53, rmcilroy wrote:
> nit - reg0_index

Done.

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:383: Register b) {
On 2016/01/04 11:54:54, rmcilroy wrote:
> nit - a->lhs  b->rhs (like .h file)

Done.

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:384: if (a != b) {
On 2016/01/04 11:54:54, rmcilroy wrote:
> Should we ever call Exchange when the registers are the same? Could this be a
> DCHECK instead?

Done.

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1189: reg.is_new_target()) {
On 2016/01/04 11:54:53, rmcilroy wrote:
> Do we ever want to allow exchanges of these registers? That would seem to be a
> bug (since the interpreter might assume the context / closure / etc. is in
these
> fixed registers).

Done.

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

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:279: os << "r" << reg.index();
On 2016/01/04 11:54:54, rmcilroy wrote:
> nit - add "a" argument case as well ?

Okay, done, was not done as not part of current changes.

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:32: V(Idx16, OperandSize::kShort)    \
On 2016/01/04 11:54:54, rmcilroy wrote:
> nit - alphabetical ordering?

Done.

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:82: V(Exchange, OperandType::kReg8,
OperandType::kReg16)                         \
On 2016/01/04 11:54:54, rmcilroy wrote:
> Is it worth having the non-wide version? It only saves a byte, and we should
> only hopefully not hit the exchange path too often (otherwise we have more to
> worry about, with the extra exchange bytecodes and the extra byte here
wouldn't
> have much impact). WDYT?

I've not found a case that would need it in the next stage of this change. Also
have not found any cases that'd benefit from ExchangeWide either and will pull
it if this continues to be the case.

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:256: static const int kMinRegisterIndex = -128;
On 2016/01/04 11:54:54, rmcilroy wrote:
> Can we get rid of these now? If not could we rename them more appropriately?

Done.

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1555713002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:211: Node* v1 = __ LoadRegister(r1);
On 2016/01/04 11:54:54, rmcilroy wrote:
> nit - non-single character variable names.

Done.

[rmcilroy, 2016-01-04 14:53:55]

lgtm

[oth, 2016-01-04 16:44:56]

The CQ bit was checked by oth@chromium.org

[oth, 2016-01-04 16:44:57]

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

[commit-bot: I haz the power, 2016-01-04 16:45:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1555713002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1555713002/80001

[commit-bot: I haz the power, 2016-01-04 17:38:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-04 17:38:35]

Description was changed from

==========
[Interpreter] Bytecodes for exchanging registers.

New bytecodes for making registers with indicies wider than 1-byte
accessible.

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

to

==========
[Interpreter] Bytecodes for exchanging registers.

New bytecodes for making registers with indicies wider than 1-byte
accessible.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/c958c98c4f11e386d4efc25da64b310e139bb62f
Cr-Commit-Position: refs/heads/master@{#33091}
==========

[commit-bot: I haz the power, 2016-01-04 17:38:36]

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