[interpreter] Add output register to ToName

[interpreter] Add output register to ToName

ToName was always generated with a subsequent Star, fuse them.
Requires a few changes in the peephole optimizer as ToName cannot be
elided as easily, but must be replaced by Star.

BUG=v8:4280
LOG=n

Committed: https://crrev.com/63ea19baa0c906e54ba18bd1311e53f9b0cc8874
Cr-Commit-Position: refs/heads/master@{#38019}

[klaasb, 2016-07-21 08:48:58]

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

[commit-bot: I haz the power, 2016-07-21 08:49:04]

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 08:54:14]

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

[commit-bot: I haz the power, 2016-07-21 08:54:14]

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

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

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

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

Similar to the ToNumber one.

@Orion: Could you please take a look at the changes to the peephole optimizer I
had to do?

@Michael: same changes to compiler/ as in the ToNumber case.

[Michael Starzinger, 2016-07-22 10:36:05]

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

https://codereview.chromium.org/2169813002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1307: void
BytecodeGraphBuilder::BuildCastOperator(const Operator* js_op) {
nit: By now the "BuildCastOperator" isn't very helpful anymore since it is only
used once. Can we remove it and just have everything inline at all three
original call sites?

[klaasb, 2016-07-22 11:00:49]

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

https://codereview.chromium.org/2169813002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1307: void
BytecodeGraphBuilder::BuildCastOperator(const Operator* js_op) {
On 2016/07/22 10:36:05, Michael Starzinger wrote:
> nit: By now the "BuildCastOperator" isn't very helpful anymore since it is
only
> used once. Can we remove it and just have everything inline at all three
> original call sites?

Sure, done.

[Michael Starzinger, 2016-07-22 11:02:55]

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

[oth, 2016-07-22 11:13:10]

Nice, LGTM with a minor comment.

https://codereview.chromium.org/2169813002/diff/60001/src/interpreter/mkpeeph...
File src/interpreter/mkpeephole.cc (right):

https://codereview.chromium.org/2169813002/diff/60001/src/interpreter/mkpeeph...
src/interpreter/mkpeephole.cc:109: // ToName optimizations: remove unnecessary
ToName bytecodes.
Can this comment be updated to reflect the new semantics of ToName and the
subsequent replacement operation with Star in the optimization here?

[rmcilroy, 2016-07-22 14:36:11]

Not had time to look fully yet, but one thought for ToObject for a later CL.

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

https://codereview.chromium.org/2169813002/diff/60001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1315: void
BytecodeGraphBuilder::VisitToObject() {
(for another CL)  -  It looks like we could probably do the same with ToObject
(it is always stored in a register in ForIn, and is stored in a register in
VisitNewLocalWithContext when called immediately after
builder()->CastAccumulatorToJSObject().  

If we do that one too then all the cast operators (apart from ToBoolean, which
is handled separately anyway) would be register output based, so we could
reinstate BuildCastOperator to do the new thing :).

[rmcilroy, 2016-07-25 10:57:47]

Thanks, LGTM.

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

https://codereview.chromium.org/2169813002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:217: // Does not update the accumulator
but stores to |out| instead.
nit - could you have two separate headings and remove the extra comments: 

// Casts accumulator and stores result in accumulator.
BytecodeArrayBuilder& CastAccumulatorToBoolean();
BytecodeArrayBuilder& CastAccumulatorToJSObject();

// Casts accumulator and stores result in register out.
BytecodeArrayBuilder& CastAccumulatorToName(Register out);
BytecodeArrayBuilder& CastAccumulatorToNumber(Register out);

https://codereview.chromium.org/2169813002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-pipeline.h (right):

https://codereview.chromium.org/2169813002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-pipeline.h:156: // replace the bytecode of this node
with |bytecode| and keep the operands
nit - Capatalize "replace" and fullstop at end of comment.

[klaasb, 2016-07-25 12:25:08]

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

https://codereview.chromium.org/2169813002/diff/60001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1315: void
BytecodeGraphBuilder::VisitToObject() {
On 2016/07/22 14:36:10, rmcilroy wrote:
> (for another CL)  -  It looks like we could probably do the same with ToObject
> (it is always stored in a register in ForIn, and is stored in a register in
> VisitNewLocalWithContext when called immediately after
> builder()->CastAccumulatorToJSObject().  
> 
> If we do that one too then all the cast operators (apart from ToBoolean, which
> is handled separately anyway) would be register output based, so we could
> reinstate BuildCastOperator to do the new thing :).

Will keep that in mind.

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

https://codereview.chromium.org/2169813002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:217: // Does not update the accumulator
but stores to |out| instead.
On 2016/07/25 10:57:47, rmcilroy wrote:
> nit - could you have two separate headings and remove the extra comments: 
> 
> // Casts accumulator and stores result in accumulator.
> BytecodeArrayBuilder& CastAccumulatorToBoolean();
> BytecodeArrayBuilder& CastAccumulatorToJSObject();
> 
> // Casts accumulator and stores result in register out.
> BytecodeArrayBuilder& CastAccumulatorToName(Register out);
> BytecodeArrayBuilder& CastAccumulatorToNumber(Register out);

Done.

https://codereview.chromium.org/2169813002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-pipeline.h (right):

https://codereview.chromium.org/2169813002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-pipeline.h:156: // replace the bytecode of this node
with |bytecode| and keep the operands
On 2016/07/25 10:57:47, rmcilroy wrote:
> nit - Capatalize "replace" and fullstop at end of comment.

Done.

https://codereview.chromium.org/2169813002/diff/60001/src/interpreter/mkpeeph...
File src/interpreter/mkpeephole.cc (right):

https://codereview.chromium.org/2169813002/diff/60001/src/interpreter/mkpeeph...
src/interpreter/mkpeephole.cc:109: // ToName optimizations: remove unnecessary
ToName bytecodes.
On 2016/07/22 11:13:10, oth wrote:
> Can this comment be updated to reflect the new semantics of ToName and the
> subsequent replacement operation with Star in the optimization here?

Done.

[klaasb, 2016-07-25 12:25:22]

The CQ bit was checked by klaasb@google.com

[klaasb, 2016-07-25 12:25:23]

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

[commit-bot: I haz the power, 2016-07-25 12:25:26]

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-25 12:51:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-25 12:53:05]

Description was changed from

==========
[interpreter] Add output register to ToName

ToName was always generated with a subsequent Star, fuse them.
Requires a few changes in the peephole optimizer as ToName cannot be
elided as easily, but must be replaced by Star.

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

to

==========
[interpreter] Add output register to ToName

ToName was always generated with a subsequent Star, fuse them.
Requires a few changes in the peephole optimizer as ToName cannot be
elided as easily, but must be replaced by Star.

BUG=v8:4280
LOG=n

Committed: https://crrev.com/63ea19baa0c906e54ba18bd1311e53f9b0cc8874
Cr-Commit-Position: refs/heads/master@{#38019}
==========

[commit-bot: I haz the power, 2016-07-25 12:53:06]

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