[Interpreter] Adds logical and, logical or and comma operators to interpreter

[Interpreter] Adds logical and/or and comma operators to interpreter

Adds support for following operators
 -Logical and
 -Logical or
 -Comma

Adds the above bytecodes, support to BytecodeGenerator and BytecodeArrayBuilder
to enable it's use, it's implementation and tests.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/5a09f1b9a776b680a4dac581f37d4c76ebd19118
Cr-Commit-Position: refs/heads/master@{#31281}

[mythria, 2015-10-09 16:08:35]

mythria@chromium.org changed reviewers:
+ bmeurer@chromium.org, rmcilroy@chromium.org

[mythria, 2015-10-09 16:08:36]

Hi,

I have added support for logical and, logical or and Comma operators to the
interpreter. Could you please review my changes.

Thanks and Regards,
Mythri

[rmcilroy, 2015-10-12 07:24:13]

https://codereview.chromium.org/1399773002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1399773002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:882: builder()->Bind(&else_label);
I think we should add a pair of bytecodes which performs ToBoolean on the
accumulator, jumps if true / false, but keeps the original value in the
accumulator. With this bytecode this sequence could be shortened to:

Visit(left);
builder()->JumpIfToBooleanTrue(&end_label);
Visit(right);
builder()->Bind(&end_label);

We could also use these bytecodes (in a future CL) to avoid having to explicitly
call ToBoolean before JumpIfTrue / False when dealing with a non-boolean test in
if / else / while conditions.

[mythria, 2015-10-13 15:00:00]

Hi,

This patch includes the following:
1. Adds a new bytecode to jump by casting the value to Boolean and updates the
bytecode generation for logical expressions. 
2. Adds additional tests for logical expressions.
3. Updates the comma operator. Earlier, there was an extra store before we visit
the right hand side expression which was not required. 
4.Rebased the patch. 

Could you please review the changes. 

Thanks and Regards,
Mythri

https://codereview.chromium.org/1399773002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1399773002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:882: builder()->Bind(&else_label);
On 2015/10/12 07:24:13, rmcilroy wrote:
> I think we should add a pair of bytecodes which performs ToBoolean on the
> accumulator, jumps if true / false, but keeps the original value in the
> accumulator. With this bytecode this sequence could be shortened to:
> 
> Visit(left);
> builder()->JumpIfToBooleanTrue(&end_label);
> Visit(right);
> builder()->Bind(&end_label);
> 
> We could also use these bytecodes (in a future CL) to avoid having to
explicitly
> call ToBoolean before JumpIfTrue / False when dealing with a non-boolean test
in
> if / else / while conditions.

Done.

[rmcilroy, 2015-10-13 15:43:24]

Looking good! A bunch of comments but most are just nits.

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

https://codereview.chromium.org/1399773002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:557: void
BytecodeGraphBuilder::VisitJumpIfToBooleanFalse(
nit - group the JumpIfToBoolean operators

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

https://codereview.chromium.org/1399773002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:127: BytecodeArrayBuilder&
JumpIfToBooleanFalse(BytecodeLabel* label);
We probably want to merge these into JumpIfTrue/False for the
bytecode-array-builder (with the array-builder choosing behind the scenes
whether to emit the jump with the ToBoolean or not), but we will do that  do
that in a followup CL - could you just add a TODO here for now.

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

https://codereview.chromium.org/1399773002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:174: bytecode == Bytecode::kJumpIfFalseConstant;
Add constant jumps here too

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

https://codereview.chromium.org/1399773002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:686: // JumpIfToBooleanTrue <imm8>
nit - group all ToBoolean versions together below

https://codereview.chromium.org/1399773002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:694: Node* toBoolean = __
CallRuntime(Runtime::kInterpreterToBoolean, accumulator);
/s/toBoolean/to_boolean_value (throughout)

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:441: {"var x = 1; var a = 2,
b = 3; return x || ("
Could you make a similar test for && as well please

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:461: B(LdaSmi8),
nit - indentation, and add '//' after

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:480: {unused, unused, unused,
unused}},
nit - would be nice to have one test that is nested use of all operators -
i.e.,: "var x = 1; return x && 3 || 0 , 1;"

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:485: {B(LdaZero), B(Return)},
nit - new bytecode on new line (use '//' to avoid git cl format 'fixing' this
for you)

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:488: {" return 1 || 3;",
nit - remove extra space at start

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:492: {B(LdaSmi8), U8(1),
B(Return)},
ditto

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:1642: TEST(InterpreterCommaSmi) {
I'm not sure these are really useful tests (also they use the old
bytecode-generator code not the new one). Personally I would make these JS code
snippet tests where you test a bunch of JS code to check the expected results.
See InterpreterArrayLiterals for examples of how to do this.

https://codereview.chromium.org/1399773002/diff/20001/test/unittests/interpre...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/1399773002/diff/20001/test/unittests/interpre...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:109:
builder.JumpIfToBooleanTrue(&start).JumpIfToBooleanFalse(&start);
nit - you can avoid the builder. and just make this a followon - i.e.:

builder.Jump(&start).JumpIfTrue(&start).JumpIfFalse(&start)
    .JumpIfToBooleanTrue(&start).JumpIfToBooleanFalse(&start);

[mythria, 2015-10-14 13:33:42]

Hi,

I modified the tests and included few additional tests for logical expressions
and Comma. I addressed all other comments from the last review. Could you please
review the changes.

Thanks and Regards,
Mythri

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

https://codereview.chromium.org/1399773002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:557: void
BytecodeGraphBuilder::VisitJumpIfToBooleanFalse(
On 2015/10/13 15:43:24, rmcilroy wrote:
> nit - group the JumpIfToBoolean operators

Done.

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

https://codereview.chromium.org/1399773002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:127: BytecodeArrayBuilder&
JumpIfToBooleanFalse(BytecodeLabel* label);
On 2015/10/13 15:43:24, rmcilroy wrote:
> We probably want to merge these into JumpIfTrue/False for the
> bytecode-array-builder (with the array-builder choosing behind the scenes
> whether to emit the jump with the ToBoolean or not), but we will do that  do
> that in a followup CL - could you just add a TODO here for now.

Done.

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

https://codereview.chromium.org/1399773002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:174: bytecode == Bytecode::kJumpIfFalseConstant;
On 2015/10/13 15:43:24, rmcilroy wrote:
> Add constant jumps here too

Done.

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

https://codereview.chromium.org/1399773002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:686: // JumpIfToBooleanTrue <imm8>
On 2015/10/13 15:43:24, rmcilroy wrote:
> nit - group all ToBoolean versions together below

Done.

https://codereview.chromium.org/1399773002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:694: Node* toBoolean = __
CallRuntime(Runtime::kInterpreterToBoolean, accumulator);
On 2015/10/13 15:43:24, rmcilroy wrote:
> /s/toBoolean/to_boolean_value (throughout)

Done.

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:441: {"var x = 1; var a = 2,
b = 3; return x || ("
On 2015/10/13 15:43:24, rmcilroy wrote:
> Could you make a similar test for && as well please

Done.

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:461: B(LdaSmi8),
On 2015/10/13 15:43:24, rmcilroy wrote:
> nit - indentation, and add '//' after

Done.

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:480: {unused, unused, unused,
unused}},
On 2015/10/13 15:43:24, rmcilroy wrote:
> nit - would be nice to have one test that is nested use of all operators -
> i.e.,: "var x = 1; return x && 3 || 0 , 1;"

Done.

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:485: {B(LdaZero), B(Return)},
On 2015/10/13 15:43:24, rmcilroy wrote:
> nit - new bytecode on new line (use '//' to avoid git cl format 'fixing' this
> for you)

Done.

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:488: {" return 1 || 3;",
On 2015/10/13 15:43:24, rmcilroy wrote:
> nit - remove extra space at start

Done.

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:492: {B(LdaSmi8), U8(1),
B(Return)},
On 2015/10/13 15:43:24, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1399773002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:1642: TEST(InterpreterCommaSmi) {
On 2015/10/13 15:43:24, rmcilroy wrote:
> I'm not sure these are really useful tests (also they use the old
> bytecode-generator code not the new one). Personally I would make these JS
code
> snippet tests where you test a bunch of JS code to check the expected results.
> See InterpreterArrayLiterals for examples of how to do this.

Done.

https://codereview.chromium.org/1399773002/diff/20001/test/unittests/interpre...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/1399773002/diff/20001/test/unittests/interpre...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:109:
builder.JumpIfToBooleanTrue(&start).JumpIfToBooleanFalse(&start);
On 2015/10/13 15:43:24, rmcilroy wrote:
> nit - you can avoid the builder. and just make this a followon - i.e.:
> 
> builder.Jump(&start).JumpIfTrue(&start).JumpIfFalse(&start)
>     .JumpIfToBooleanTrue(&start).JumpIfToBooleanFalse(&start);

Done.

[Benedikt Meurer, 2015-10-14 13:36:47]

LGTM on compiler from my side.

[rmcilroy, 2015-10-14 14:32:44]

LGTM once final nits are addressed, thanks!

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

https://codereview.chromium.org/1399773002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.cc:754: // JumpIfToBooleanTrue <imm8>
nit - move these to be below DoJumpIfFalseConstant

https://codereview.chromium.org/1399773002/diff/60001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1399773002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:412: for (size_t i = 0; i <
arraysize(snippets); i++) {
Weird indentation here?

https://codereview.chromium.org/1399773002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:520: {"var x = 1; return x &&
3 || 0,1;",
nit - /s/0,1/0, 1

https://codereview.chromium.org/1399773002/diff/60001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1399773002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:1739: std::make_pair("var a = 0, b =
10; return a && b/a;\n",
nit - b/a; -> b / a;

https://codereview.chromium.org/1399773002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:1747: std::make_pair("return 'a' &&
0 || 'b','c';\n",
nit - 'b','c'; -> 'b', 'c';

[mythria, 2015-10-14 14:59:56]

Thanks for your reviews. Addressed all nits from last review. I will land it
once the trybots are successful.

Thanks and Regards,
Mythri

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

https://codereview.chromium.org/1399773002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.cc:754: // JumpIfToBooleanTrue <imm8>
On 2015/10/14 14:32:44, rmcilroy wrote:
> nit - move these to be below DoJumpIfFalseConstant

Done.

https://codereview.chromium.org/1399773002/diff/60001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1399773002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:412: for (size_t i = 0; i <
arraysize(snippets); i++) {
On 2015/10/14 14:32:44, rmcilroy wrote:
> Weird indentation here?

Done.

https://codereview.chromium.org/1399773002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:520: {"var x = 1; return x &&
3 || 0,1;",
On 2015/10/14 14:32:44, rmcilroy wrote:
> nit - /s/0,1/0, 1

Done.

https://codereview.chromium.org/1399773002/diff/60001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1399773002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:1739: std::make_pair("var a = 0, b =
10; return a && b/a;\n",
On 2015/10/14 14:32:44, rmcilroy wrote:
> nit - b/a; -> b / a;

Done.

https://codereview.chromium.org/1399773002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:1747: std::make_pair("return 'a' &&
0 || 'b','c';\n",
On 2015/10/14 14:32:44, rmcilroy wrote:
> nit - 'b','c'; -> 'b', 'c';

Done.

[mythria, 2015-10-14 15:32:15]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-10-14 15:32:15]

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/1399773002/#ps80001
(title: "Fixed few more nits")

[commit-bot: I haz the power, 2015-10-14 15:32:31]

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

[rmcilroy, 2015-10-14 15:34:01]

https://codereview.chromium.org/1399773002/diff/80001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1399773002/diff/80001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:472: B(LdaSmi8),
Bytecode and operands on one line (I've noticed git cl format seems to barf at
these lines with the "undef X" above it).

https://codereview.chromium.org/1399773002/diff/80001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:498: B(LdaSmi8),
ditto

[rmcilroy, 2015-10-14 15:34:20]

The CQ bit was unchecked by rmcilroy@chromium.org

[mythria, 2015-10-15 09:10:15]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-10-15 09:10:16]

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/1399773002/#ps100001
(title: "Fixed formatting problems with the expected bytecode initialization")

[commit-bot: I haz the power, 2015-10-15 09:10:32]

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

[commit-bot: I haz the power, 2015-10-15 09:11:43]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-15 09:12:08]

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