[Interpreter] Add support for for..in.

[Interpreter] Add support for for..in.

For..in introduces 3 new bytecodes ForInPrepare, ForInNext, and
ForInDone to start a for..in loop, get the next element, and check if
the loop is done.

For..in builds upon new LoopBuilder constructs for conditionally
breaking and continuing during iteration: BreakIf{Null|Undefined}
and ContinueIf{Null|Undefined}. New conditional jump bytecodes
support this succinctly: JumpIfNull and JumpIfUndefined.

Add missing check to BytecodeLabel that could allow multiple
forward referencess to the same label which is not supported.

BUG=v8:4280
LOG=N

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

[oth, 2015-10-26 12:03:07]

Description was changed from

==========
[Interpreter] Add support for for..in.

Remove x64 debugging in code-stubs-x64.cc.

For..in introduces 3 new bytecodes ForInPrepare, ForInNext, and
ForInDone to start a for..in loop, get the next element, and check if
the loop is done. Each bytecode is a call to the runtime.

Add BreakIfTrue, ContinueIfTrue to IfBuilder.

Add missing check to BytecodeLabel that could allow multiple
forward references s to the same label which is not supported.

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

to

==========
[Interpreter] Add support for for..in.

For..in introduces 3 new bytecodes ForInPrepare, ForInNext, and
ForInDone to start a for..in loop, get the next element, and check if
the loop is done. Each bytecode is a call to the runtime.

Add BreakIfTrue, ContinueIfTrue to IfBuilder.

Add missing check to BytecodeLabel that could allow multiple
forward references s to the same label which is not supported.

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

[oth, 2015-10-26 12:10:50]

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

[oth, 2015-10-26 12:10:50]

verwaest@chromium.org: Please review changes in runtime. Do you have any
thoughts on the duplication of GetPropertyNames fast? There don't seem to be any
other runtime calls that depend on runtime calls as InterpreterForInPrepare
does.

rmcilroy@chromium.org: Please review changes in intepreter,
interpreter-assembler, and tests.

bmeurer@chromium.org: Please review changes in runtime-interpreter.cc,
particularly logic in Runtime_InterpreterForInPrepare.

Thanks.

[Benedikt Meurer, 2015-10-26 12:17:37]

https://codereview.chromium.org/1422033002/diff/20001/src/runtime/runtime-int...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1422033002/diff/20001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:176:
RUNTIME_FUNCTION(Runtime_InterpreterForInPrepare) {
Is this is a temporary thing? Because this runtime function will not do the
trick, as we need to be able to lazy deopt after the ToObject for example, or
the GetPropertyNamesFast.

[rmcilroy, 2015-10-26 14:04:01]

Looks pretty good. A couple more comments.

https://codereview.chromium.org/1422033002/diff/20001/src/compiler/interprete...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1422033002/diff/20001/src/compiler/interprete...
src/compiler/interpreter-assembler.cc:294: Node*
InterpreterAssembler::LoadFixedArrayElement(Node* fixed_array,
nit - please add a test for this in interpreter-assembler-unittests.cc

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:612: Register value =
temporary_register_scope.NewRegister();
nit - use expression_result()->NewRegister() ?

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:623: Register value =
temporary_register_scope.NewRegister();
ditto

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:640: EffectResultScope result_scope(this);
Is this needed? Shouldn't we just be using VisitForEffect where necessary below?

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:656: Register for_in_state =
temporary_register_scope.NewRegister();
nit - move this up (and use execution_result()->NewRegister ?

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:657:
builder()->StoreAccumulatorInRegister(for_in_state);
nit - use builder chaining?

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:660: Strength::WEAK);
/s/Strength::WEAK/language_mode_strength()

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:663: // The loop
nit -full stop

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:675: // Get next item
nit - full stop

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:682: builder()->LoadUndefined();
Would it be worth maintaining a temprorary register with Undefined in it for the
duration of the loop - that would allow this to become: 

builder()->ForInNext(for_in_state);
   .CompareOperation(Token::Value::EQ_STRICT, undefined,
language_mode_strength())
loop_builder.ContinueIfTrue();
...

In fact, if we add a new JumpIfUndefined bytecode this could all roll into:
builder()->ForInNext(for_in_state);
loop_builder.ContinueIfUndefined();
VisitForInAssignment(stmt->each(), stmt->EachFeedbackSlot());

With no need to move the value in / out of registers. WDYT?

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:696:
builder()->BinaryOperation(Token::Value::ADD, index, Strength::WEAK);
You could use builder()->CountOperation(Token::Value::ADD, index, ...), which
will emit Inc and avoid the LoadLiteral()

Also /s/Strength::WEAK/language_mode_strength();

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

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:146: /* For..in */                                  
                             \
nit - move Throw and Return below these

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

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1086: // ForInNext <for_in_prepared>
/s/for_in_prepared/for_in_state

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1091: // invocation of ForInPrepare.
remove extra newline

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1111: // invocation of ForInPrepare.
ditto

[oth, 2015-10-28 11:53:12]

Thanks, comments incorporated. PTAL.

https://codereview.chromium.org/1422033002/diff/20001/src/compiler/interprete...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1422033002/diff/20001/src/compiler/interprete...
src/compiler/interpreter-assembler.cc:294: Node*
InterpreterAssembler::LoadFixedArrayElement(Node* fixed_array,
On 2015/10/26 14:04:01, rmcilroy wrote:
> nit - please add a test for this in interpreter-assembler-unittests.cc

Done.

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:612: Register value =
temporary_register_scope.NewRegister();
On 2015/10/26 14:04:01, rmcilroy wrote:
> nit - use expression_result()->NewRegister() ?

This would cause us to allocate an extra register in VisitForIn, whereas if this
is local it doesn't increase the frame size.

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:623: Register value =
temporary_register_scope.NewRegister();
On 2015/10/26 14:04:01, rmcilroy wrote:
> ditto

This would cause us to allocate an extra register in VisitForIn, whereas if this
is local it doesn't increase the frame size.

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:640: EffectResultScope result_scope(this);
On 2015/10/26 14:04:01, rmcilroy wrote:
> Is this needed? Shouldn't we just be using VisitForEffect where necessary
below?

This is needed. RegisterResultScope can allocate a register in the outer()
execution result which is null without this at present. Added a comment about
the StatementResultScope which'd obviate the need for this.

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:656: Register for_in_state =
temporary_register_scope.NewRegister();
On 2015/10/26 14:04:01, rmcilroy wrote:
> nit - move this up (and use execution_result()->NewRegister ? 

Done.

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:657:
builder()->StoreAccumulatorInRegister(for_in_state);
On 2015/10/26 14:04:01, rmcilroy wrote:
> nit - use builder chaining?

Done-ish. Changed to chain in blocks that are logical units.

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:660: Strength::WEAK);
On 2015/10/26 14:04:01, rmcilroy wrote:
> /s/Strength::WEAK/language_mode_strength()

Done.

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:663: // The loop
On 2015/10/26 14:04:01, rmcilroy wrote:
> nit -full stop 

Done.

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:675: // Get next item
On 2015/10/26 14:04:01, rmcilroy wrote:
> nit - full stop

Done.

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:682: builder()->LoadUndefined();
On 2015/10/26 14:04:01, rmcilroy wrote:
> Would it be worth maintaining a temprorary register with Undefined in it for
the
> duration of the loop - that would allow this to become: 
> 
> builder()->ForInNext(for_in_state);
>    .CompareOperation(Token::Value::EQ_STRICT, undefined,
> language_mode_strength())
> loop_builder.ContinueIfTrue();
> ...
> 
> In fact, if we add a new JumpIfUndefined bytecode this could all roll into:
> builder()->ForInNext(for_in_state);
> loop_builder.ContinueIfUndefined();
> VisitForInAssignment(stmt->each(), stmt->EachFeedbackSlot());
> 
> With no need to move the value in / out of registers. WDYT?

This is superb! Done.

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:696:
builder()->BinaryOperation(Token::Value::ADD, index, Strength::WEAK);
On 2015/10/26 14:04:01, rmcilroy wrote:
> You could use builder()->CountOperation(Token::Value::ADD, index, ...), which
> will emit Inc and avoid the LoadLiteral()
> 
> Also /s/Strength::WEAK/language_mode_strength();

Done.

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

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:146: /* For..in */                                  
                             \
On 2015/10/26 14:04:01, rmcilroy wrote:
> nit - move Throw and Return below these

Done.

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

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1086: // ForInNext <for_in_prepared>
On 2015/10/26 14:04:01, rmcilroy wrote:
> /s/for_in_prepared/for_in_state

Done.

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1091: // invocation of ForInPrepare.
On 2015/10/26 14:04:01, rmcilroy wrote:
> remove extra newline

Done.

https://codereview.chromium.org/1422033002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1111: // invocation of ForInPrepare.
On 2015/10/26 14:04:01, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1422033002/diff/20001/src/runtime/runtime-int...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1422033002/diff/20001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:176:
RUNTIME_FUNCTION(Runtime_InterpreterForInPrepare) {
On 2015/10/26 12:17:37, Benedikt Meurer wrote:
> Is this is a temporary thing? Because this runtime function will not do the
> trick, as we need to be able to lazy deopt after the ToObject for example, or
> the GetPropertyNamesFast.

Yes, good points - I was not thinking about this. The revised code calls the
runtime for GetPropertyNamesFast at the bytecode level and uses a ToObject
bytecode.

[Benedikt Meurer, 2015-10-28 12:03:31]

LGTM for runtime-interpreter.cc and bytecode choice.

https://codereview.chromium.org/1422033002/diff/20001/src/runtime/runtime-int...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1422033002/diff/20001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:176:
RUNTIME_FUNCTION(Runtime_InterpreterForInPrepare) {
Ack. Thanks.

[rmcilroy, 2015-10-28 13:46:23]

lgtm once comments are addressed.

https://codereview.chromium.org/1422033002/diff/100001/src/compiler/interpret...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1422033002/diff/100001/src/compiler/interpret...
src/compiler/interpreter-assembler.h:83: Node* UndefinedConstant();
nit - you can just use HeapConstant(factory()->undefined_value()) for these to
avoid the extra functions. I don't really mind though (although would we used
one or the other approach consistently in interpreter.cc).

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

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:267: // previous bytecode stored the
accumulator with the same register.
Same could apply for StoreAccumulatorInRegister couldn't it?

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

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:662: // CL in preparation will a
StatementResultScope that will remove
/s/will a Statement.../will add a Statement.../

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:686: Register for_in_state =
execution_result()->NewRegister();
nit - I would put Register declarations at the top of the (whitespace
deliniated) code blocks where they are first used. Just personal preference
though, so feel free to ignore me.

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:691: language_mode_strength());
Can you use BreakIfUndefined here instead?

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:696: Register index = receiver;  //
re-using register as receiver no longer used.
nit - /s/re/Re

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:707:
builder()->LoadAccumulatorWithRegister(index).ForInNext(for_in_state);
nit - could you require ForInNext to take a register for the index to avoid
having to do LoadAccumulatorWithRegister here (one less byte, and one less
dispatch)?

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:716: Visit(stmt->body());
VisitForEffect?

https://codereview.chromium.org/1422033002/diff/100001/test/cctest/interprete...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1422033002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:2542: // TODO(oth): Add a test here
for delete mid-loop when delete is ready.
it's now ready and landed ;)

[rmcilroy, 2015-10-28 17:20:27]

On 2015/10/28 13:46:23, rmcilroy wrote:
> lgtm once comments are addressed.
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/compiler/interpret...
> File src/compiler/interpreter-assembler.h (right):
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/compiler/interpret...
> src/compiler/interpreter-assembler.h:83: Node* UndefinedConstant();
> nit - you can just use HeapConstant(factory()->undefined_value()) for these to
> avoid the extra functions. I don't really mind though (although would we used
> one or the other approach consistently in interpreter.cc).
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> File src/interpreter/bytecode-array-builder.cc (right):
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-array-builder.cc:267: // previous bytecode stored the
> accumulator with the same register.
> Same could apply for StoreAccumulatorInRegister couldn't it?
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> File src/interpreter/bytecode-generator.cc (right):
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:662: // CL in preparation will a
> StatementResultScope that will remove
> /s/will a Statement.../will add a Statement.../
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:686: Register for_in_state =
> execution_result()->NewRegister();
> nit - I would put Register declarations at the top of the (whitespace
> deliniated) code blocks where they are first used. Just personal preference
> though, so feel free to ignore me.
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:691: language_mode_strength());
> Can you use BreakIfUndefined here instead?
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:696: Register index = receiver;  //
> re-using register as receiver no longer used.
> nit - /s/re/Re
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:707:
> builder()->LoadAccumulatorWithRegister(index).ForInNext(for_in_state);
> nit - could you require ForInNext to take a register for the index to avoid
> having to do LoadAccumulatorWithRegister here (one less byte, and one less
> dispatch)?
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:716: Visit(stmt->body());
> VisitForEffect?
> 
>
https://codereview.chromium.org/1422033002/diff/100001/test/cctest/interprete...
> File test/cctest/interpreter/test-interpreter.cc (right):
> 
>
https://codereview.chromium.org/1422033002/diff/100001/test/cctest/interprete...
> test/cctest/interpreter/test-interpreter.cc:2542: // TODO(oth): Add a test
here
> for delete mid-loop when delete is ready.
> it's now ready and landed ;)

Also, please update the description and /s/references s/references/

[oth, 2015-10-28 22:49:16]

Description was changed from

==========
[Interpreter] Add support for for..in.

For..in introduces 3 new bytecodes ForInPrepare, ForInNext, and
ForInDone to start a for..in loop, get the next element, and check if
the loop is done. Each bytecode is a call to the runtime.

Add BreakIfTrue, ContinueIfTrue to IfBuilder.

Add missing check to BytecodeLabel that could allow multiple
forward references s to the same label which is not supported.

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

to

==========
[Interpreter] Add support for for..in.

For..in introduces 3 new bytecodes ForInPrepare, ForInNext, and
ForInDone to start a for..in loop, get the next element, and check if
the loop is done.

For..in builds upon new LoopBuilder constructs for conditionally
breaking and continuing during iteration: BreakIf{Null|Undefined}
and ContinueIf{Null|Undefined}. New conditional jump bytecodes
support this succinctly: JumpIfNull and JumpIfUndefined.

Add missing check to BytecodeLabel that could allow multiple
forward referencess to the same label which is not supported.

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

[oth, 2015-10-28 22:50:40]

Thanks, comments incorporated.

https://codereview.chromium.org/1422033002/diff/100001/src/compiler/interpret...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1422033002/diff/100001/src/compiler/interpret...
src/compiler/interpreter-assembler.h:83: Node* UndefinedConstant();
On 2015/10/28 13:46:22, rmcilroy wrote:
> nit - you can just use HeapConstant(factory()->undefined_value()) for these to
> avoid the extra functions. I don't really mind though (although would we used
> one or the other approach consistently in interpreter.cc).

Okay, used the more verbose form.

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

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:267: // previous bytecode stored the
accumulator with the same register.
On 2015/10/28 13:46:22, rmcilroy wrote:
> Same could apply for StoreAccumulatorInRegister couldn't it?

Comment added.

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

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:662: // CL in preparation will a
StatementResultScope that will remove
On 2015/10/28 13:46:22, rmcilroy wrote:
> /s/will a Statement.../will add a Statement.../

Done.

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:686: Register for_in_state =
execution_result()->NewRegister();
On 2015/10/28 13:46:22, rmcilroy wrote:
> nit - I would put Register declarations at the top of the (whitespace
> deliniated) code blocks where they are first used. Just personal preference
> though, so feel free to ignore me.

Done.

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:691: language_mode_strength());
On 2015/10/28 13:46:22, rmcilroy wrote:
> Can you use BreakIfUndefined here instead?

Done.

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:696: Register index = receiver;  //
re-using register as receiver no longer used.
On 2015/10/28 13:46:22, rmcilroy wrote:
> nit - /s/re/Re

Done.

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:707:
builder()->LoadAccumulatorWithRegister(index).ForInNext(for_in_state);
On 2015/10/28 13:46:22, rmcilroy wrote:
> nit - could you require ForInNext to take a register for the index to avoid
> having to do LoadAccumulatorWithRegister here (one less byte, and one less
> dispatch)?

Done.

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:716: Visit(stmt->body());
On 2015/10/28 13:46:22, rmcilroy wrote:
> VisitForEffect?

stmt->body() is a statement rather than expression so not applicable here.

https://codereview.chromium.org/1422033002/diff/100001/test/cctest/interprete...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1422033002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:2542: // TODO(oth): Add a test here
for delete mid-loop when delete is ready.
On 2015/10/28 13:46:22, rmcilroy wrote:
> it's now ready and landed ;)

Done.

[oth, 2015-10-29 06:07:58]

On 2015/10/28 22:50:40, oth wrote:
> Thanks, comments incorporated.
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/compiler/interpret...
> File src/compiler/interpreter-assembler.h (right):
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/compiler/interpret...
> src/compiler/interpreter-assembler.h:83: Node* UndefinedConstant();
> On 2015/10/28 13:46:22, rmcilroy wrote:
> > nit - you can just use HeapConstant(factory()->undefined_value()) for these
to
> > avoid the extra functions. I don't really mind though (although would we
used
> > one or the other approach consistently in interpreter.cc).
> 
> Okay, used the more verbose form.
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> File src/interpreter/bytecode-array-builder.cc (right):
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-array-builder.cc:267: // previous bytecode stored the
> accumulator with the same register.
> On 2015/10/28 13:46:22, rmcilroy wrote:
> > Same could apply for StoreAccumulatorInRegister couldn't it?
> 
> Comment added.
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> File src/interpreter/bytecode-generator.cc (right):
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:662: // CL in preparation will a
> StatementResultScope that will remove
> On 2015/10/28 13:46:22, rmcilroy wrote:
> > /s/will a Statement.../will add a Statement.../
> 
> Done.
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:686: Register for_in_state =
> execution_result()->NewRegister();
> On 2015/10/28 13:46:22, rmcilroy wrote:
> > nit - I would put Register declarations at the top of the (whitespace
> > deliniated) code blocks where they are first used. Just personal preference
> > though, so feel free to ignore me.
> 
> Done.
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:691: language_mode_strength());
> On 2015/10/28 13:46:22, rmcilroy wrote:
> > Can you use BreakIfUndefined here instead?
> 
> Done.
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:696: Register index = receiver;  //
> re-using register as receiver no longer used.
> On 2015/10/28 13:46:22, rmcilroy wrote:
> > nit - /s/re/Re
> 
> Done.
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:707:
> builder()->LoadAccumulatorWithRegister(index).ForInNext(for_in_state);
> On 2015/10/28 13:46:22, rmcilroy wrote:
> > nit - could you require ForInNext to take a register for the index to avoid
> > having to do LoadAccumulatorWithRegister here (one less byte, and one less
> > dispatch)?
> 
> Done.
> 
>
https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:716: Visit(stmt->body());
> On 2015/10/28 13:46:22, rmcilroy wrote:
> > VisitForEffect?
> 
> stmt->body() is a statement rather than expression so not applicable here.
> 
>
https://codereview.chromium.org/1422033002/diff/100001/test/cctest/interprete...
> File test/cctest/interpreter/test-interpreter.cc (right):
> 
>
https://codereview.chromium.org/1422033002/diff/100001/test/cctest/interprete...
> test/cctest/interpreter/test-interpreter.cc:2542: // TODO(oth): Add a test
here
> for delete mid-loop when delete is ready.
> On 2015/10/28 13:46:22, rmcilroy wrote:
> > it's now ready and landed ;)
> 
> Done.

Ross, can you take another look and confirm the changes are okay. Thanks.

[rmcilroy, 2015-10-29 10:53:45]

LGTM, thanks!

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

https://codereview.chromium.org/1422033002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:716: Visit(stmt->body());
On 2015/10/28 22:50:40, oth wrote:
> On 2015/10/28 13:46:22, rmcilroy wrote:
> > VisitForEffect?
> 
> stmt->body() is a statement rather than expression so not applicable here.

Acknowledged.

https://codereview.chromium.org/1422033002/diff/140001/test/cctest/interprete...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1422033002/diff/140001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:2457: "return sloppy_b;",
Thanks for fixing this!

[oth, 2015-10-29 11:47:25]

The CQ bit was checked by oth@chromium.org

[oth, 2015-10-29 11:47:26]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1422033002/#ps140001
(title: "Comment nits.")

[commit-bot: I haz the power, 2015-10-29 11:47:32]

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

[commit-bot: I haz the power, 2015-10-29 12:06:04]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-29 12:06:32]

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