[Interpreter] Basic flow control.

[Interpreter] Basic flow control.

+ Add bytecodes for conditional and unconditional jumps.
+ Add bytecodes for test/compare operations.
+ Expose jumps in bytecode-array-builder and add BytecodeLabel class for
  identifying jump targets.
+ Add support for if..then...else in the bytecode-generator.
+ Implement jump bytecodes in the interpreter. Test/compare operations
  dependent on runtime call for comparisons.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/347fa90626a448e3535cf6aa100124dfd5b711ce
Cr-Commit-Position: refs/heads/master@{#30918}

[oth, 2015-09-17 14:18:33]

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

[oth, 2015-09-17 14:18:33]

PTAL, this is the start of flow control. Thanks!

[Benedikt Meurer, 2015-09-17 17:04:17]

bmeurer@chromium.org changed required reviewers:
+ mstarzinger@chromium.org, rmcilroy@chromium.org

[Benedikt Meurer, 2015-09-17 17:04:18]

LGTM on the basic approach from me. Two nits.

https://codereview.chromium.org/1343363002/diff/120001/src/compiler/interpret...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1343363002/diff/120001/src/compiler/interpret...
src/compiler/interpreter-assembler.cc:366: void
InterpreterAssembler::JumpIfEqual(Node* lhs, Node* rhs, Node* delta) {
Nit: Can you rename that to JumpIfWordEqual or something like that, which makes
it obvious from the name which kind of equality test is being performed?

https://codereview.chromium.org/1343363002/diff/120001/src/compiler/interpret...
src/compiler/interpreter-assembler.cc:377: void
InterpreterAssembler::JumpIfNotEqual(Node* lhs, Node* rhs, Node* delta) {
Same here.

https://codereview.chromium.org/1343363002/diff/120001/src/compiler/interpret...
src/compiler/interpreter-assembler.cc:431: DCHECK(end_nodes_.size() > 0);
Nit: Use !end_nodes_.empty()

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

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:127: builder().JumpIfFalse(&else_start);
Oh thank god, this is so much better to just materialize the stupid true/false
and compare against that in the if, than this micro optimization with the (most
often untagged) raw integer for the compare in fullcodegen. I think I'll try to
fix that in fullcodegen as well, so it is uniform. But this alone will avoid
some hairy tricks that we need to place to properly deopt to fullcodegen
currently.

[rmcilroy, 2015-09-18 10:42:24]

Look good overall. My one main issue is with the way Output is generalized to do
patching, but otherwise the approach looks good.

https://codereview.chromium.org/1343363002/diff/120001/src/compiler/interpret...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1343363002/diff/120001/src/compiler/interpret...
src/compiler/interpreter-assembler.cc:377: void
InterpreterAssembler::JumpIfNotEqual(Node* lhs, Node* rhs, Node* delta) {
Looks like you don't use this yet, let's just remove it for now.

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

https://codereview.chromium.org/1343363002/diff/120001/src/compiler/interpret...
src/compiler/interpreter-assembler.h:112: void Jump(Node* jump_offset);
please add tests for these in interpreter-assembler-unittests please.

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

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:89: if (bytecodes()->end() == pos) {
I'm not keen on having the Output operators do patching automatically based on
position. I think Output should always push to the end of the bytecode array
(like it did previously) and there should be a special Patch function for
patching a given bytecode - that way we can also verify that we are performing a
valid patch (i.e., the previous bytecode is the same size as the patching
bytecode, etc). Given that this is likely only going to be used for forward
branches I think it makes sense to make it more special to avoid accidental
misuse.

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

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:47: BytecodeArrayBuilder&
LoadLiteral(int value);
I would prefer not to add this since it's not clear if it will end up being a
smi or a heapnumber. It looks like it is only for tests - could you just do
LoadLiteral(Smi::FromInt(v)) in the tests where you need this?

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

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:123: // sequence of test operations.
Could you add the ToBoolean unconditionally for now (making the interpreter nop
this for now like the comparison ops is fine, but I'd like the bytecode
generated to be correct).

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:477: // TODO(oth): ast-graph-builder has
language_mode() option for LT/LE/etc.
Could you pass the language mode here and do UNIMPLEMENTED for everything except
SLOPPY in bytecode-array-builder (like the Load/StoreICs) - just to make sure we
don't miss this later and have subtle bugs.

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecodes.h:74: V(JumpSmi8, OperandType::kImm8)                 
                        \
This is not really a Smi, it is just an imm8. How about we just remove the
suffix from these (e.g., Jump, JumpIfTrue, etc.) since they are expected to be
the normal case?

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecodes.h:76: V(JumpIfTrueSmi8, OperandType::kImm8)           
                        \
As discussed, you aren't using JumpIfTrue yet except for tests. Could you add a
TODO here to remove it if we don't need it after implementing the other control
flow constructs.

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecodes.h:79: V(JumpIfFalseConstant, OperandType::kIdx)       
                        \
optional super nit - I would prefer ordered by operand type, i.e.:
Jump
JumpIfTrue
JumpIfFalse
JumpConstant...

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

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.cc:352: // Test if register equals the accumulator.
nit (for all comments) - /s/Test if register/Test if value in <src> register

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.cc:368: // Test if register strictly equal to the
accumulator.
/s/strictly/is strictly (and below)

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.cc:438: Node* int_value = __ BytecodeOperandImm8(0);
nit - /s/int_value/jump_delta (and below)

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

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.h:57: // Generates code to perform the comparison
operation associated with .
finish comment :).

https://codereview.chromium.org/1343363002/diff/120001/test/cctest/interprete...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1343363002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:617: "function f(arg1) { if
(false) { return 1; } else { return -1; } }",
nit - remove arg1

https://codereview.chromium.org/1343363002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:648: }},
could you add some tests for:
 - non-bool values (e.g., if (arg1)...
 - a couple of other comparison operations (e.g., ===, instanceof, in, etc.)
 - code which would require a constant pool jump (e.g., >128 bytecode range)
(you could do this as a separate test which doesn't require you to write out 128
bytecodes to check in the snippet, but maybe you only check the prefix of the
resulting bytecode)

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

https://codereview.chromium.org/1343363002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:899:
CHECK_EQ(Smi::cast(*return_value)->value(), 7);
Nice tests :).

[Michael Starzinger, 2015-09-23 08:46:24]

Looking good, mainly nits and random ramblings.

https://codereview.chromium.org/1343363002/diff/220001/src/compiler/interpret...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1343363002/diff/220001/src/compiler/interpret...
src/compiler/interpreter-assembler.cc:371: DispatchTo(Advance(delta));
Just a random comment, nothing actionable: This creates a handler with two
dispatch-sites, totally valid. Another way of modeling this would be to use just
one dispatch-site and conditionally adapt the bytecode-PC. Essentially the value
flowing into {DispatchTo} would be a {Phi} in that case. Not sure which approach
is better, either is fine with me, just wanted to throw it out there.

https://codereview.chromium.org/1343363002/diff/220001/src/compiler/interpret...
src/compiler/interpreter-assembler.cc:414: DCHECK(input);
nit: DCHECK_NOT_NULL instead.

https://codereview.chromium.org/1343363002/diff/220001/src/compiler/raw-machi...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1343363002/diff/220001/src/compiler/raw-machi...
src/compiler/raw-machine-assembler.h:108: Handle<HeapObject> boolean_value =
nit: Just use Factory::ToBoolean(bool) here.

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

https://codereview.chromium.org/1343363002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:488: UNIMPLEMENTED();
nit: This should actually be UNREACHABLE(), right?

https://codereview.chromium.org/1343363002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:518: UNIMPLEMENTED();
nit: This should actually be UNREACHABLE(), right?

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

https://codereview.chromium.org/1343363002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:126: // when logical connectives are
implemented, e.g. &&, ||, etc. Optimization
I am not sure what this TODO refers to exactly, but I think the coercion is in
the correct position here. The evaluation order of [[ToBoolean]] coercions
within if-statements and and binary logical operators is fully specified.
Short-circuiting for example can provide uncoerced value of {c} for {a || b ||
c}. So in general the if-statement needs to coerce before testing.

[Michael Starzinger, 2015-09-23 09:24:27]

LGTM.

[oth, 2015-09-23 10:46:56]

Thanks all! Definitely looking better.

Will revise JumpIfWordEqual per mstarzinger's suggestion and update test before
call for final comments.

https://codereview.chromium.org/1343363002/diff/120001/src/compiler/interpret...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1343363002/diff/120001/src/compiler/interpret...
src/compiler/interpreter-assembler.cc:366: void
InterpreterAssembler::JumpIfEqual(Node* lhs, Node* rhs, Node* delta) {
On 2015/09/17 17:04:17, Benedikt Meurer wrote:
> Nit: Can you rename that to JumpIfWordEqual or something like that, which
makes
> it obvious from the name which kind of equality test is being performed?

Done.

https://codereview.chromium.org/1343363002/diff/120001/src/compiler/interpret...
src/compiler/interpreter-assembler.cc:377: void
InterpreterAssembler::JumpIfNotEqual(Node* lhs, Node* rhs, Node* delta) {
On 2015/09/17 17:04:17, Benedikt Meurer wrote:
> Same here.

Done.

https://codereview.chromium.org/1343363002/diff/120001/src/compiler/interpret...
src/compiler/interpreter-assembler.cc:377: void
InterpreterAssembler::JumpIfNotEqual(Node* lhs, Node* rhs, Node* delta) {
On 2015/09/17 17:04:17, Benedikt Meurer wrote:
> Same here.

Acknowledged.

https://codereview.chromium.org/1343363002/diff/120001/src/compiler/interpret...
src/compiler/interpreter-assembler.cc:431: DCHECK(end_nodes_.size() > 0);
On 2015/09/17 17:04:17, Benedikt Meurer wrote:
> Nit: Use !end_nodes_.empty()

Done.

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

https://codereview.chromium.org/1343363002/diff/120001/src/compiler/interpret...
src/compiler/interpreter-assembler.h:112: void Jump(Node* jump_offset);
On 2015/09/18 10:42:23, rmcilroy wrote:
> please add tests for these in interpreter-assembler-unittests please.

Added a test for Jump and a partial test JumpIfWordEqual. Going to try
mstarzinger's approach to JumpIfWordEqual first and then revise the latter test.

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

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:89: if (bytecodes()->end() == pos) {
On 2015/09/18 10:42:23, rmcilroy wrote:
> I'm not keen on having the Output operators do patching automatically based on
> position. I think Output should always push to the end of the bytecode array
> (like it did previously) and there should be a special Patch function for
> patching a given bytecode - that way we can also verify that we are performing
a
> valid patch (i.e., the previous bytecode is the same size as the patching
> bytecode, etc). Given that this is likely only going to be used for forward
> branches I think it makes sense to make it more special to avoid accidental
> misuse.

Done.

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

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:47: BytecodeArrayBuilder&
LoadLiteral(int value);
On 2015/09/18 10:42:23, rmcilroy wrote:
> I would prefer not to add this since it's not clear if it will end up being a
> smi or a heapnumber. It looks like it is only for tests - could you just do
> LoadLiteral(Smi::FromInt(v)) in the tests where you need this?

Done. The intent was to enable writing slightly cleaner tests. Ditto the
Add/Subtract/Multiple/Divide/Modulo change (which is also dropped).

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

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:123: // sequence of test operations.
On 2015/09/18 10:42:23, rmcilroy wrote:
> Could you add the ToBoolean unconditionally for now (making the interpreter
nop
> this for now like the comparison ops is fine, but I'd like the bytecode
> generated to be correct).

Done.

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:477: // TODO(oth): ast-graph-builder has
language_mode() option for LT/LE/etc.
On 2015/09/18 10:42:23, rmcilroy wrote:
> Could you pass the language mode here and do UNIMPLEMENTED for everything
except
> SLOPPY in bytecode-array-builder (like the Load/StoreICs) - just to make sure
we
> don't miss this later and have subtle bugs.

Done.

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecodes.h:74: V(JumpSmi8, OperandType::kImm8)                 
                        \
On 2015/09/18 10:42:23, rmcilroy wrote:
> This is not really a Smi, it is just an imm8. How about we just remove the
> suffix from these (e.g., Jump, JumpIfTrue, etc.) since they are expected to be
> the normal case?

Done.

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecodes.h:76: V(JumpIfTrueSmi8, OperandType::kImm8)           
                        \
On 2015/09/18 10:42:24, rmcilroy wrote:
> As discussed, you aren't using JumpIfTrue yet except for tests. Could you add
a
> TODO here to remove it if we don't need it after implementing the other
control
> flow constructs.

do...while() is a prime candidate for JumpIfTrue.

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecodes.h:79: V(JumpIfFalseConstant, OperandType::kIdx)       
                        \
On 2015/09/18 10:42:23, rmcilroy wrote:
> optional super nit - I would prefer ordered by operand type, i.e.:
> Jump
> JumpIfTrue
> JumpIfFalse
> JumpConstant...

The current ordering groups by functionality like many asm references. The code
here doesn't have to match that, but I'd prefer not to churn the declaration and
implementation in this CL.

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

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.cc:352: // Test if register equals the accumulator.
On 2015/09/18 10:42:24, rmcilroy wrote:
> nit (for all comments) - /s/Test if register/Test if value in <src> register

Done.

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.cc:368: // Test if register strictly equal to the
accumulator.
On 2015/09/18 10:42:24, rmcilroy wrote:
> /s/strictly/is strictly (and below)

Done.

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.cc:438: Node* int_value = __ BytecodeOperandImm8(0);
On 2015/09/18 10:42:24, rmcilroy wrote:
> nit - /s/int_value/jump_delta (and below)

Done. s/int_value/relative_jump/ to match code further on.

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

https://codereview.chromium.org/1343363002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.h:57: // Generates code to perform the comparison
operation associated with .
On 2015/09/18 10:42:24, rmcilroy wrote:
> finish comment :).

Done.

https://codereview.chromium.org/1343363002/diff/120001/test/cctest/interprete...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1343363002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:617: "function f(arg1) { if
(false) { return 1; } else { return -1; } }",
On 2015/09/18 10:42:24, rmcilroy wrote:
> nit - remove arg1

Done.

https://codereview.chromium.org/1343363002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:648: }},
On 2015/09/18 10:42:24, rmcilroy wrote:
> could you add some tests for:
>  - non-bool values (e.g., if (arg1)...
>  - a couple of other comparison operations (e.g., ===, instanceof, in, etc.)
>  - code which would require a constant pool jump (e.g., >128 bytecode range)
> (you could do this as a separate test which doesn't require you to write out
128
> bytecodes to check in the snippet, but maybe you only check the prefix of the
> resulting bytecode)

Done. There will be more tests to follow as well in the test operator
implementation CL.

https://codereview.chromium.org/1343363002/diff/220001/src/compiler/interpret...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1343363002/diff/220001/src/compiler/interpret...
src/compiler/interpreter-assembler.cc:371: DispatchTo(Advance(delta));
On 2015/09/23 08:46:24, Michael Starzinger wrote:
> Just a random comment, nothing actionable: This creates a handler with two
> dispatch-sites, totally valid. Another way of modeling this would be to use
just
> one dispatch-site and conditionally adapt the bytecode-PC. Essentially the
value
> flowing into {DispatchTo} would be a {Phi} in that case. Not sure which
approach
> is better, either is fine with me, just wanted to throw it out there.

This suggestion is better than what we have (at least the code seems too large).
Will try this before committing.

https://codereview.chromium.org/1343363002/diff/220001/src/compiler/interpret...
src/compiler/interpreter-assembler.cc:414: DCHECK(input);
On 2015/09/23 08:46:24, Michael Starzinger wrote:
> nit: DCHECK_NOT_NULL instead.

Done.

https://codereview.chromium.org/1343363002/diff/220001/src/compiler/raw-machi...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1343363002/diff/220001/src/compiler/raw-machi...
src/compiler/raw-machine-assembler.h:108: Handle<HeapObject> boolean_value =
On 2015/09/23 08:46:24, Michael Starzinger wrote:
> nit: Just use Factory::ToBoolean(bool) here.

Done.

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

https://codereview.chromium.org/1343363002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:488: UNIMPLEMENTED();
On 2015/09/23 08:46:24, Michael Starzinger wrote:
> nit: This should actually be UNREACHABLE(), right?

Done.

https://codereview.chromium.org/1343363002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:518: UNIMPLEMENTED();
On 2015/09/23 08:46:24, Michael Starzinger wrote:
> nit: This should actually be UNREACHABLE(), right?

Done.

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

https://codereview.chromium.org/1343363002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:126: // when logical connectives are
implemented, e.g. &&, ||, etc. Optimization
On 2015/09/23 08:46:24, Michael Starzinger wrote:
> I am not sure what this TODO refers to exactly, but I think the coercion is in
> the correct position here. The evaluation order of [[ToBoolean]] coercions
> within if-statements and and binary logical operators is fully specified.
> Short-circuiting for example can provide uncoerced value of {c} for {a || b ||
> c}. So in general the if-statement needs to coerce before testing.

Sorry about the wording here.

Basically, the goal would be that BytecodeArrayBuilder is more selective about
whether it emits the CastToBoolean bytecode. Specifically, if the previous
bytecode is in the same block and that bytecode would have put a boolean value
in the accumulator then CastToBoolean is not emitted.

Implemented this in patch set 16.

[rmcilroy, 2015-09-23 14:00:29]

Looks great. A bunch of comments but most are just nits.

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

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:53: EnsureReturn();
nit - newlines around this.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:88: Output(bytes);
personally I prefer the old-style returns (each of which just did the
push_back() themselves) - this avoided building a new array for each bytecode as
these do.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:270: DCHECK(false);
nit - just UNREACHABLE()

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:304: bool
BytecodeArrayBuilder::IsJumpWithSmi8Operand(Bytecode jump_bytecode) {
nit /s/Smi8/Imm8

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:342: size_t entry =
GetConstantPoolEntry(handle(Smi::FromInt(delta), isolate()));
We will need to reserve the constant pool entry ahead of time (when the jump is
emitted) to ensure it the index will fit in a byte when we get to patch it. We
can do this in a followup CL, but please ad the TODO in the else block to remind
us.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:383: }
nit - could we fold OutputJump into this function - i.e.:

  int delta;
  if (label->is_bound()) {
    delta = bytecodes()->size() - label->offset();
  } else {
    label->set_referrer(bytecodes()->size());
    delta = 0;
  }
 
  if (FitsInImm8Operand(delta)) {  ...

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:420: while (offset <
bytecodes()->size()) {
As discussed offline, please add a TODO to avoid this while loop when we track
the blocks better.

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

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:120: void EnsureReturn();
nit - newline above EnsureReturn();

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.h (right):

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.h:28: int8_t GetSmi8Operand(int
operand_index) const;
Just noticed, this should really be GetImm8Operand.

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

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:112: // For control-flow it could be
useful to signal empty paths here.
nit - TODO(oth):

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:117: // TODO(oth): spot easy cases if
(true/1/false/0)?
nit - clarify comment please - e.g.: spot easy cases where we wouldn't need to
emit the then or else block, e.g., if (true/1/false/0).

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:126: builder().Bind(&else_end);
nit - maybe some whitespace between condition, then, else, etc.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecodes.h:74: V(CastToBoolean, OperandType::kNone)            
                        \
Drop the "Cast" just ToBoolean (thoughout), since this is the name of the
operator specified in the ES spec.

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

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/interp...
src/interpreter/interpreter.cc:285: void Interpreter::DoCompareOp(Token::Value
op,
nit - add header comment above function.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/interp...
src/interpreter/interpreter.cc:475: Node* boolean_value = __
BooleanConstant(true);
nit - boolean_value / true_value

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/interp...
src/interpreter/interpreter.cc:490: Node* boolean_value = __
BooleanConstant(true);
ditto

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/interp...
src/interpreter/interpreter.cc:502: Node* boolean_value = __
BooleanConstant(false);
false_value

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/interp...
src/interpreter/interpreter.cc:517: Node* boolean_value = __
BooleanConstant(false);
ditto

https://codereview.chromium.org/1343363002/diff/280001/test/cctest/interprete...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1343363002/diff/280001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:624: // a valid target.
Is the todo done?

https://codereview.chromium.org/1343363002/diff/280001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:625: B(LdaSmi8), U8(-1),
B(Return), B(LdaUndefined), B(Return)}},
nit - I know git cl format messes this format up, but could you put each
bytecode on it's own line like the other tests

https://codereview.chromium.org/1343363002/diff/280001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:691: };
could you add the expected constant pool entry here too please.

[Michael Starzinger, 2015-09-24 10:37:22]

https://codereview.chromium.org/1343363002/diff/320001/src/compiler/raw-machi...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1343363002/diff/320001/src/compiler/raw-machi...
src/compiler/raw-machine-assembler.h:108: return
HeapConstant(handle(HeapObject::cast(*object), isolate()));
nit: Better use Handle<HeapObject>::cast(object) to avoid allocating a new
handle location.

[oth, 2015-09-24 11:15:28]

Thanks all. Still missing refinement the test for JumpIf, but getting closer.

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

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:53: EnsureReturn();
On 2015/09/23 14:00:28, rmcilroy wrote:
> nit - newlines around this.

Done.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:88: Output(bytes);
On 2015/09/23 14:00:28, rmcilroy wrote:
> personally I prefer the old-style returns (each of which just did the
> push_back() themselves) - this avoided building a new array for each bytecode
as
> these do.

The point is well made. This pattern occurs elsewhere in v8 and I think there's
a good case centralizing this:

1) Fewer calls to std::vector::push_back() and means fewer resize checks and
fewer resizes.
2) Common code is not repeated and potentially lost when next iteration is
cut-n-pasted or new functionality added, like the update of
last_bytecode_start_.
3) No fiddly handwritten hard-coded constants.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:270: DCHECK(false);
On 2015/09/23 14:00:28, rmcilroy wrote:
> nit - just UNREACHABLE()

Done.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:304: bool
BytecodeArrayBuilder::IsJumpWithSmi8Operand(Bytecode jump_bytecode) {
On 2015/09/23 14:00:28, rmcilroy wrote:
> nit /s/Smi8/Imm8

Done.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:342: size_t entry =
GetConstantPoolEntry(handle(Smi::FromInt(delta), isolate()));
On 2015/09/23 14:00:28, rmcilroy wrote:
> We will need to reserve the constant pool entry ahead of time (when the jump
is
> emitted) to ensure it the index will fit in a byte when we get to patch it. We
> can do this in a followup CL, but please ad the TODO in the else block to
remind
> us.

Done.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:383: }
On 2015/09/23 14:00:28, rmcilroy wrote:
> nit - could we fold OutputJump into this function - i.e.:
> 
>   int delta;
>   if (label->is_bound()) {
>     delta = bytecodes()->size() - label->offset();
>   } else {
>     label->set_referrer(bytecodes()->size());
>     delta = 0;
>   }
>  
>   if (FitsInImm8Operand(delta)) {  ...

Done.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:420: while (offset <
bytecodes()->size()) {
On 2015/09/23 14:00:28, rmcilroy wrote:
> As discussed offline, please add a TODO to avoid this while loop when we track
> the blocks better.

Switched to O(1) implementation.

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

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:120: void EnsureReturn();
On 2015/09/23 14:00:28, rmcilroy wrote:
> nit - newline above EnsureReturn();

Done.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.h (right):

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.h:28: int8_t GetSmi8Operand(int
operand_index) const;
On 2015/09/23 14:00:28, rmcilroy wrote:
> Just noticed, this should really be GetImm8Operand.

Done.

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

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:112: // For control-flow it could be
useful to signal empty paths here.
On 2015/09/23 14:00:29, rmcilroy wrote:
> nit - TODO(oth): 

Done.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:117: // TODO(oth): spot easy cases if
(true/1/false/0)?
On 2015/09/23 14:00:29, rmcilroy wrote:
> nit - clarify comment please - e.g.: spot easy cases where we wouldn't need to
> emit the then or else block, e.g., if (true/1/false/0).

Done.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:126: builder().Bind(&else_end);
On 2015/09/23 14:00:29, rmcilroy wrote:
> nit - maybe some whitespace between condition, then, else, etc.

Done.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecodes.h:74: V(CastToBoolean, OperandType::kNone)            
                        \
On 2015/09/23 14:00:29, rmcilroy wrote:
> Drop the "Cast" just ToBoolean (thoughout), since this is the name of the
> operator specified in the ES spec.

Done.

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

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/interp...
src/interpreter/interpreter.cc:285: void Interpreter::DoCompareOp(Token::Value
op,
On 2015/09/23 14:00:29, rmcilroy wrote:
> nit - add header comment above function.

This method is not exposed externally and like other methods in this file does
not have a header. In the follow on CL for test operators, the method doesn't
exist.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/interp...
src/interpreter/interpreter.cc:475: Node* boolean_value = __
BooleanConstant(true);
On 2015/09/23 14:00:29, rmcilroy wrote:
> nit - boolean_value / true_value

Done.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/interp...
src/interpreter/interpreter.cc:490: Node* boolean_value = __
BooleanConstant(true);
On 2015/09/23 14:00:29, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/interp...
src/interpreter/interpreter.cc:502: Node* boolean_value = __
BooleanConstant(false);
On 2015/09/23 14:00:29, rmcilroy wrote:
> false_value

Done.

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/interp...
src/interpreter/interpreter.cc:517: Node* boolean_value = __
BooleanConstant(false);
On 2015/09/23 14:00:29, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1343363002/diff/280001/test/cctest/interprete...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1343363002/diff/280001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:624: // a valid target.
On 2015/09/23 14:00:29, rmcilroy wrote:
> Is the todo done?

No, there needs to be code to evaluate whether the statement blocks are empty.

https://codereview.chromium.org/1343363002/diff/280001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:625: B(LdaSmi8), U8(-1),
B(Return), B(LdaUndefined), B(Return)}},
On 2015/09/23 14:00:29, rmcilroy wrote:
> nit - I know git cl format messes this format up, but could you put each
> bytecode on it's own line like the other tests

Done.

https://codereview.chromium.org/1343363002/diff/280001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:691: };
On 2015/09/23 14:00:29, rmcilroy wrote:
> could you add the expected constant pool entry here too please.

Done.

https://codereview.chromium.org/1343363002/diff/320001/src/compiler/raw-machi...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1343363002/diff/320001/src/compiler/raw-machi...
src/compiler/raw-machine-assembler.h:108: return
HeapConstant(handle(HeapObject::cast(*object), isolate()));
On 2015/09/24 10:37:22, Michael Starzinger wrote:
> nit: Better use Handle<HeapObject>::cast(object) to avoid allocating a new
> handle location.

Done. Thank you, I was groping for the right path here.

[rmcilroy, 2015-09-24 11:44:36]

LGTM! Thanks!

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

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:88: Output(bytes);
On 2015/09/24 11:15:27, oth wrote:
> On 2015/09/23 14:00:28, rmcilroy wrote:
> > personally I prefer the old-style returns (each of which just did the
> > push_back() themselves) - this avoided building a new array for each
bytecode
> as
> > these do.
> 
> The point is well made. This pattern occurs elsewhere in v8 and I think
there's
> a good case centralizing this:
> 
> 1) Fewer calls to std::vector::push_back() and means fewer resize checks and
> fewer resizes.
> 2) Common code is not repeated and potentially lost when next iteration is
> cut-n-pasted or new functionality added, like the update of
> last_bytecode_start_.
> 3) No fiddly handwritten hard-coded constants.
> 

Ok, you've convinced me :).

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

https://codereview.chromium.org/1343363002/diff/280001/src/interpreter/interp...
src/interpreter/interpreter.cc:285: void Interpreter::DoCompareOp(Token::Value
op,
On 2015/09/24 11:15:27, oth wrote:
> On 2015/09/23 14:00:29, rmcilroy wrote:
> > nit - add header comment above function.
> 
> This method is not exposed externally and like other methods in this file does
> not have a header. In the follow on CL for test operators, the method doesn't
> exist.

Right, sorry - forgot this was the generic version.

https://codereview.chromium.org/1343363002/diff/280001/test/cctest/interprete...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1343363002/diff/280001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:624: // a valid target.
On 2015/09/24 11:15:27, oth wrote:
> On 2015/09/23 14:00:29, rmcilroy wrote:
> > Is the todo done?
> 
> No, there needs to be code to evaluate whether the statement blocks are empty.

Right - I was confused about what the comment meant, but in clarifying the
comment I see it's to do with the jump being dead (not the valid target which is
fixed) so this is fine.

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

https://codereview.chromium.org/1343363002/diff/380001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:92: BytecodeArrayBuilder& EnterBlock();
nit - comment

https://codereview.chromium.org/1343363002/diff/380001/test/cctest/interprete...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1343363002/diff/380001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:225: {1234}}};
was this reformat intended?

https://codereview.chromium.org/1343363002/diff/380001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:641: B(Return)               
                                //
Thanks for fixing all these other tests!

[oth, 2015-09-24 14:50:53]

The CQ bit was checked by oth@chromium.org

[oth, 2015-09-24 14:50:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, mstarzinger@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1343363002/#ps420001
(title: "Switch test-bytecode-generator/IfConditions to use new style bytecode
array check.")

[commit-bot: I haz the power, 2015-09-24 14:51:03]

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

[commit-bot: I haz the power, 2015-09-24 15:20:53]

Committed patchset #22 (id:420001)

[commit-bot: I haz the power, 2015-09-24 15:21:18]

Patchset 22 (id:??) landed as
https://crrev.com/347fa90626a448e3535cf6aa100124dfd5b711ce
Cr-Commit-Position: refs/heads/master@{#30918}