[interpreter] Add a register operand to ToNumber

[interpreter] Add a register operand to ToNumber

ToNumber's result is always directly stored to a register using a Star
bytecode. Fuse it into ToNumber.

BUG=v8:4280
LOG=n

Committed: https://crrev.com/220284638e719b67d586da10983e2006942c7040
Cr-Commit-Position: refs/heads/master@{#37976}

[klaasb, 2016-07-20 14:34:50]

klaasb@google.com changed reviewers:
+ rmcilroy@chromium.org

[klaasb, 2016-07-20 14:34:50]

Please take a look. Performance try bot results on the first patch set,
basically unchanged - some speedups for arm32 possible to imagine.

[klaasb, 2016-07-20 16:33:14]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 16:33:19]

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-07-20 16:37:31]

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

[commit-bot: I haz the power, 2016-07-20 16:37:32]

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/9398)

[klaasb, 2016-07-20 16:44:25]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 16:44: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, 2016-07-20 16:58:01]

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

[commit-bot: I haz the power, 2016-07-20 16:58:01]

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

[klaasb, 2016-07-21 08:46:55]

Description was changed from

==========
[interpreter] Add a register operand to ToNumber

ToNumber's result is always directly stored to a register using a Star
bytecode. Fuse it into ToNumber.

BUG=
LOG=n
==========

to

==========
[interpreter] Add a register operand to ToNumber

ToNumber's result is always directly stored to a register using a Star
bytecode. Fuse it into ToNumber.

BUG=v8:4280
LOG=n
==========

[rmcilroy, 2016-07-21 09:31:48]

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

https://codereview.chromium.org/2165953002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:218: BytecodeArrayBuilder&
CastAccumulatorToNumber(Register out);
Can you add a comment that this doesn't update the accumulator

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

https://codereview.chromium.org/2165953002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:215: V(ToNumber, AccumulatorUse::kRead,
OperandType::kRegOut)                     \
You need to modify BytecodeGraphBuilder as well, otherwise optimized code will
break. 

Do no tests break with this change? If so, could you add something to
test-run-bytecode-graph-builder which would break until you update it.

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

https://codereview.chromium.org/2165953002/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1007: if (Bytecodes::NumberOfOperands(bytecode)
== 1 &&
Let's not do this in an if here. Split out BuildUnaryOp which only does the
actual operation, and call that form DoUnaryOp and also seperately from ToNumber
and modify ToNumber to store in the register and dispatch.

[klaasb, 2016-07-21 10:55:19]

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

https://codereview.chromium.org/2165953002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:218: BytecodeArrayBuilder&
CastAccumulatorToNumber(Register out);
On 2016/07/21 09:31:48, rmcilroy wrote:
> Can you add a comment that this doesn't update the accumulator

Done.

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

https://codereview.chromium.org/2165953002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:215: V(ToNumber, AccumulatorUse::kRead,
OperandType::kRegOut)                     \
On 2016/07/21 09:31:48, rmcilroy wrote:
> You need to modify BytecodeGraphBuilder as well, otherwise optimized code will
> break. 

Done.

> Do no tests break with this change?

Some did actually, sorry.

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

https://codereview.chromium.org/2165953002/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1007: if (Bytecodes::NumberOfOperands(bytecode)
== 1 &&
On 2016/07/21 09:31:48, rmcilroy wrote:
> Let's not do this in an if here. Split out BuildUnaryOp which only does the
> actual operation, and call that form DoUnaryOp and also seperately from
ToNumber
> and modify ToNumber to store in the register and dispatch.

Done.

[klaasb, 2016-07-21 14:25:57]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 14:25:59]

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-07-21 15:14:02]

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

[commit-bot: I haz the power, 2016-07-21 15:14:03]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-07-22 09:36:36]

LGTM. You will need to get a compiler/ OWNERS review as well.

https://codereview.chromium.org/2165953002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2165953002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:218: // Does not update the accumulator
but stores to the register operand instead.
/s/the register operand/|out|/

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

https://codereview.chromium.org/2165953002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.h:84: InterpreterAssembler* assembler);
nit move down with the other Build* functions

[klaasb, 2016-07-22 09:45:44]

klaasb@google.com changed reviewers:
+ mstarzinger@chromium.org

[klaasb, 2016-07-22 09:45:45]

Hi Michael,

could you please take a look as well?

https://codereview.chromium.org/2165953002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2165953002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:218: // Does not update the accumulator
but stores to the register operand instead.
On 2016/07/22 09:36:36, rmcilroy wrote:
> /s/the register operand/|out|/

Done.

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

https://codereview.chromium.org/2165953002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.h:84: InterpreterAssembler* assembler);
On 2016/07/22 09:36:36, rmcilroy wrote:
> nit move down with the other Build* functions

Done.

[Michael Starzinger, 2016-07-22 09:50:02]

LGTM on "compiler", didn't look at the rest.

[klaasb, 2016-07-22 10:01:07]

The CQ bit was checked by klaasb@google.com

[klaasb, 2016-07-22 10:01:07]

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/2165953002/#ps120001
(title: "comments")

[commit-bot: I haz the power, 2016-07-22 10:01:13]

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-07-22 10:24:06]

Description was changed from

==========
[interpreter] Add a register operand to ToNumber

ToNumber's result is always directly stored to a register using a Star
bytecode. Fuse it into ToNumber.

BUG=v8:4280
LOG=n
==========

to

==========
[interpreter] Add a register operand to ToNumber

ToNumber's result is always directly stored to a register using a Star
bytecode. Fuse it into ToNumber.

BUG=v8:4280
LOG=n
==========

[commit-bot: I haz the power, 2016-07-22 10:24:07]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-22 10:26:17]

Description was changed from

==========
[interpreter] Add a register operand to ToNumber

ToNumber's result is always directly stored to a register using a Star
bytecode. Fuse it into ToNumber.

BUG=v8:4280
LOG=n
==========

to

==========
[interpreter] Add a register operand to ToNumber

ToNumber's result is always directly stored to a register using a Star
bytecode. Fuse it into ToNumber.

BUG=v8:4280
LOG=n

Committed: https://crrev.com/220284638e719b67d586da10983e2006942c7040
Cr-Commit-Position: refs/heads/master@{#37976}
==========

[commit-bot: I haz the power, 2016-07-22 10:26:18]

Patchset 7 (id:??) landed as
https://crrev.com/220284638e719b67d586da10983e2006942c7040
Cr-Commit-Position: refs/heads/master@{#37976}