[interpreter] Bytecode for StaDataPropertyInLiteral.

[interpreter] Bytecode for StaDataPropertyInLiteral.

Add bytecode for defining data properties, which initially just calls the runtime function.

BUG=v8:5624
Committed: https://crrev.com/f7795cbf6e5be1e04536ed5380e2f46767344e2b
Cr-Commit-Position: refs/heads/master@{#41101}

[Franzi, 2016-11-16 15:48:31]

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

[commit-bot: I haz the power, 2016-11-16 15:48:42]

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

[Franzi, 2016-11-16 15:54:34]

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

[commit-bot: I haz the power, 2016-11-16 15:54:44]

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

[Franzi, 2016-11-16 15:59:01]

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

[commit-bot: I haz the power, 2016-11-16 15:59:13]

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-11-16 16:16:56]

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

[commit-bot: I haz the power, 2016-11-16 16:16:59]

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

[Franzi, 2016-11-16 16:50:53]

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

[commit-bot: I haz the power, 2016-11-16 16:51:03]

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-11-16 16:56:59]

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

[commit-bot: I haz the power, 2016-11-16 16:57:00]

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...)

[Franzi, 2016-11-17 10:52:24]

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

[commit-bot: I haz the power, 2016-11-17 10:52:41]

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-11-17 11:06:45]

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

[commit-bot: I haz the power, 2016-11-17 11:06:46]

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...)

[Franzi, 2016-11-17 13:34:25]

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

[commit-bot: I haz the power, 2016-11-17 13:34:30]

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-11-17 13:49:00]

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

[commit-bot: I haz the power, 2016-11-17 13:49: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...)

[Franzi, 2016-11-17 15:41:10]

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

[commit-bot: I haz the power, 2016-11-17 15:41:14]

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-11-17 15:46:44]

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

[commit-bot: I haz the power, 2016-11-17 15:46:45]

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...)

[Franzi, 2016-11-17 15:49:40]

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

[commit-bot: I haz the power, 2016-11-17 15:49:44]

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

[Franzi, 2016-11-17 15:50:57]

Description was changed from

==========
[interpreter] Bytecode for StaDataPropertyInLiteral.

BUG=v8:5624
==========

to

==========
[interpreter] Bytecode for StaDataPropertyInLiteral.

Add bytecode for defining data properties, which initially just calls the
runtime function.

BUG=v8:5624
==========

[Franzi, 2016-11-17 15:50:57]

franzih@chromium.org changed reviewers:
+ bmeurer@chromium.org, leszeks@chromium.org

[Franzi, 2016-11-17 15:58:06]

Please take a look. Thanks, 

Franzi

[commit-bot: I haz the power, 2016-11-17 16:17:07]

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

[commit-bot: I haz the power, 2016-11-17 16:17:08]

Dry run: This issue passed the CQ dry run.

[Leszek Swirski, 2016-11-17 16:58:47]

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

[Leszek Swirski, 2016-11-17 16:58:47]

leszeks@chromium.org changed required reviewers:
+ rmcilroy@chromium.org

[Leszek Swirski, 2016-11-17 16:58:48]

Seems reasonable to me, adding Ross to reviewers in case I missed something.

https://codereview.chromium.org/2510743002/diff/180001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2510743002/diff/180001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1484:
.StoreAccumulatorInRegister(set_function_name);
you don't need this twice.

[Franzi, 2016-11-17 17:00:53]

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

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

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

[Franzi, 2016-11-17 17:06:10]

https://codereview.chromium.org/2510743002/diff/180001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2510743002/diff/180001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1484:
.StoreAccumulatorInRegister(set_function_name);
On 2016/11/17 16:58:48, Leszek Swirski wrote:
> you don't need this twice.

Good catch! Thanks.

[rmcilroy, 2016-11-17 17:18:17]

https://codereview.chromium.org/2510743002/diff/200001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2510743002/diff/200001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:803:
environment()->BindAccumulator(store, Environment::kAttachFrameState);
You shouldn't be calling BindAccumulator here since the bytecode doesn't write
to the accumulator. I think what you want here is RecordAfterState()

https://codereview.chromium.org/2510743002/diff/200001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2510743002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1484:
builder()->LoadAccumulatorWithRegister(value);
nit - rather than having value to be in the accumulator, how about just having
set_function_name be in the accumulator and keep value in a register, that way
the bytecode sequence could be:
 ->LoadLiteral(Smi::FromInt(property->NeedsSetFunctionName()))
  .StoreDataPropertyInLiteral(receiver, key, attr, value);

which would save two extra bytecodes.

https://codereview.chromium.org/2510743002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1485:
builder()->StoreDataPropertyInLiteral(receiver, key, attr,
nit - just chain the builder calls together like those above.

https://codereview.chromium.org/2510743002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1704: RegisterList args =
register_allocator()->NewRegisterList(5);
These no longer need to be a sequence of consecutive registers (since you aren't
doing a call). This allows you to avoid fixing the registers into a sequence and
having to move "literal" into a different register.
I.e., you can now just do the following:

Register attr = register_allocator()->NewRegister();
Register key = register_allocator()->NewRegister();

VisitForAccumulatorValue(property->key());
builder()->ConvertAccumulatorToName(key)
Register value = VisitForRegisterValue(property->value());
VisitSetHomeObject(value, literal, property);
builder()->LoadLiteral(Smi::FromInt(NONE))
   .StoreAccumulatorInRegister(attr)
   .LoadLiteral(Smi::FromInt(property->NeedsSetFunctionName()))
   .StoreDataPropertyInLiteral(literal, key, attr, value);

https://codereview.chromium.org/2510743002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1716:
builder()->StoreDataPropertyInLiteral(args[0], args[1], args[3],
ditto

[commit-bot: I haz the power, 2016-11-17 17:34:49]

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

[commit-bot: I haz the power, 2016-11-17 17:34:50]

Dry run: This issue passed the CQ dry run.

[Franzi, 2016-11-18 08:29:47]

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

[commit-bot: I haz the power, 2016-11-18 08:29:56]

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-11-18 08:35:07]

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

[commit-bot: I haz the power, 2016-11-18 08:35:08]

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...)

[Franzi, 2016-11-18 08:37:20]

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

[commit-bot: I haz the power, 2016-11-18 08:37:27]

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-11-18 09:02:29]

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

[commit-bot: I haz the power, 2016-11-18 09:02:30]

Dry run: This issue passed the CQ dry run.

[Franzi, 2016-11-18 09:11:53]

Ross, thanks for the detailed review. Please take another look. 

Franzi

https://codereview.chromium.org/2510743002/diff/200001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2510743002/diff/200001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:803:
environment()->BindAccumulator(store, Environment::kAttachFrameState);
On 2016/11/17 17:18:16, rmcilroy wrote:
> You shouldn't be calling BindAccumulator here since the bytecode doesn't write
> to the accumulator. I think what you want here is RecordAfterState()

Done.

https://codereview.chromium.org/2510743002/diff/200001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2510743002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1484:
builder()->LoadAccumulatorWithRegister(value);
On 2016/11/17 17:18:17, rmcilroy wrote:
> nit - rather than having value to be in the accumulator, how about just having
> set_function_name be in the accumulator and keep value in a register, that way
> the bytecode sequence could be:
>  ->LoadLiteral(Smi::FromInt(property->NeedsSetFunctionName()))
>   .StoreDataPropertyInLiteral(receiver, key, attr, value);
> 
> which would save two extra bytecodes.

Done.

https://codereview.chromium.org/2510743002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1485:
builder()->StoreDataPropertyInLiteral(receiver, key, attr,
On 2016/11/17 17:18:17, rmcilroy wrote:
> nit - just chain the builder calls together like those above.

Done.

https://codereview.chromium.org/2510743002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1704: RegisterList args =
register_allocator()->NewRegisterList(5);
On 2016/11/17 17:18:17, rmcilroy wrote:
> These no longer need to be a sequence of consecutive registers (since you
aren't
> doing a call). This allows you to avoid fixing the registers into a sequence
and
> having to move "literal" into a different register.
> I.e., you can now just do the following:
> 
> Register attr = register_allocator()->NewRegister();
> Register key = register_allocator()->NewRegister();
> 
> VisitForAccumulatorValue(property->key());
> builder()->ConvertAccumulatorToName(key)
> Register value = VisitForRegisterValue(property->value());
> VisitSetHomeObject(value, literal, property);
> builder()->LoadLiteral(Smi::FromInt(NONE))
>    .StoreAccumulatorInRegister(attr)
>    .LoadLiteral(Smi::FromInt(property->NeedsSetFunctionName()))
>    .StoreDataPropertyInLiteral(literal, key, attr, value);

Done.

https://codereview.chromium.org/2510743002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1716:
builder()->StoreDataPropertyInLiteral(args[0], args[1], args[3],
On 2016/11/17 17:18:17, rmcilroy wrote:
> ditto

Done.

[Benedikt Meurer, 2016-11-18 09:17:39]

compiler part LGTM.

[rmcilroy, 2016-11-18 09:33:50]

LGTM, thanks.

https://codereview.chromium.org/2510743002/diff/280001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (left):

https://codereview.chromium.org/2510743002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1489: args.Truncate(4));
Thanks for fixing this too!

https://codereview.chromium.org/2510743002/diff/280001/src/interpreter/interp...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2510743002/diff/280001/src/interpreter/interp...
src/interpreter/interpreter.cc:807: void
Interpreter::DoStaDataPropertyInLiteral(InterpreterAssembler* assembler) {
Nit - please add a comment describing what the bytecode does and the operands
(like StaNamedProperyStrict above). Also could you move this below the keyed
store properly bytecodes to keep named / keyed grouped.

[Franzi, 2016-11-18 10:16:29]

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

[commit-bot: I haz the power, 2016-11-18 10:16:43]

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-11-18 11:43:19]

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

[commit-bot: I haz the power, 2016-11-18 11:43:20]

Dry run: This issue passed the CQ dry run.

[Franzi, 2016-11-18 12:11:29]

The CQ bit was checked by franzih@chromium.org

[Franzi, 2016-11-18 12:11:29]

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/2510743002/#ps300001
(title: "Add comment and move interpreter function.")

[commit-bot: I haz the power, 2016-11-18 12:11:34]

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-11-18 12:13:36]

Description was changed from

==========
[interpreter] Bytecode for StaDataPropertyInLiteral.

Add bytecode for defining data properties, which initially just calls the
runtime function.

BUG=v8:5624
==========

to

==========
[interpreter] Bytecode for StaDataPropertyInLiteral.

Add bytecode for defining data properties, which initially just calls the
runtime function.

BUG=v8:5624
==========

[commit-bot: I haz the power, 2016-11-18 12:13:38]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-11-18 12:13:57]

Description was changed from

==========
[interpreter] Bytecode for StaDataPropertyInLiteral.

Add bytecode for defining data properties, which initially just calls the
runtime function.

BUG=v8:5624
==========

to

==========
[interpreter] Bytecode for StaDataPropertyInLiteral.

Add bytecode for defining data properties, which initially just calls the
runtime function.

BUG=v8:5624

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

[commit-bot: I haz the power, 2016-11-18 12:13:58]

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