[Interpreter] Add support for cast operators to bytecode graph builder and

[Interpreter] Add support for cast operators to bytecode graph builder and
an optomization to remove redundant cast operations.

1. Adds an optimization to remove redundant ToBoolean and ToName operations.
2. Adds implementation and tests for cast operatorts to bytecode graph builder.

BUG=v8:4280
LOG=N

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

[mythria, 2015-11-23 10:54:02]

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

[mythria, 2015-11-23 10:54:02]

Could you please review my changes.

Thanks,
Mythri

[oth, 2015-11-23 13:46:41]

Overall this looks good. Minor niggle with the naming - it needs to be clearer.

One option would be to have a PreviousBytecodeHelper that BytecodeArrayBuilder
used for these operations:

previous_bytecode()->GetBytecode()
previous_bytecode()->GetOperand()
previous_bytecode()->GetConstantForIndexOperand()

WDYT?

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

https://codereview.chromium.org/1468003002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1003: Node* node =
There's a common pattern here and also in VisitLogicalNot/VisitTypeOf so perhaps
an idea to have a common method VisitAccumulatorOperator(const Operation* op)
that all these methods use.

https://codereview.chromium.org/1468003002/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1468003002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:1031: Bytecode
BytecodeArrayBuilder::GetPreviousBytecodeInSameBlock() const {
Suggest renaming to GetPreviousBytecode() as the method works if previous
bytecode is not in the same block and the name is pretty long otherwise.

https://codereview.chromium.org/1468003002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:1041: uint32_t
BytecodeArrayBuilder::GetRawOperand(int operand_index) const {
For clarity, it would be better to call this something like:

GetOperandOfPreviousBytecode().

https://codereview.chromium.org/1468003002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:1063: Handle<Object>
BytecodeArrayBuilder::GetConstantForIndexOperand(
GetConstantForIndexOperandOfPreviousBytecode(). Phew! Seems pretty long...

[mythria, 2015-11-24 12:58:48]

Could you please review my changes. 

Thanks,
Mythri

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

https://codereview.chromium.org/1468003002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1003: Node* node =
On 2015/11/23 13:46:41, oth wrote:
> There's a common pattern here and also in VisitLogicalNot/VisitTypeOf so
perhaps
> an idea to have a common method VisitAccumulatorOperator(const Operation* op)
> that all these methods use.

I added a method for cast operators. I did not include
VisitLogicalNot/VisitTypeOf because they do not take any frame state inputs.

https://codereview.chromium.org/1468003002/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1468003002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:1031: Bytecode
BytecodeArrayBuilder::GetPreviousBytecodeInSameBlock() const {
On 2015/11/23 13:46:41, oth wrote:
> Suggest renaming to GetPreviousBytecode() as the method works if previous
> bytecode is not in the same block and the name is pretty long otherwise.

I added a helper class that takes BytecodeArrayBuilder. I could also change it
to take more specific information it needs: last_bytecode_start_,
last_block_end_ and bytecode array. I am not sure which is better. Any
suggestions?

[oth, 2015-11-24 13:49:26]

A couple of nits, but lgtm. thanks!

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

https://codereview.chromium.org/1468003002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:13: explicit
PreviousBytecodeHelper(BytecodeArrayBuilder& array_builder)
The array_builder argument and member should be const:
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

https://codereview.chromium.org/1468003002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:54: BytecodeArrayBuilder&
array_builder;
Coding style is for members of a class to end with underscore.

[mythria, 2015-11-24 14:33:06]

Thanks orion, I fixed both of them.

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

https://codereview.chromium.org/1468003002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:13: explicit
PreviousBytecodeHelper(BytecodeArrayBuilder& array_builder)
On 2015/11/24 13:49:26, oth wrote:
> The array_builder argument and member should be const:
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments

Done.

https://codereview.chromium.org/1468003002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:54: BytecodeArrayBuilder&
array_builder;
On 2015/11/24 13:49:26, oth wrote:
> Coding style is for members of a class to end with underscore.

Done.

[Benedikt Meurer, 2015-11-30 10:05:12]

LGTM on compiler.

[mythria, 2015-11-30 12:21:50]

Description was changed from

==========
[Interpreter] Add support for cast operators to bytecode graph builder and
an optomization to remove redundant cast operations.

1. Adds an optimization to remove redundant ToBoolean and ToName operations.
2. Adds implementation and tests for cast operatorts to bytecode graph builder.
3. Adds unit tests for bytecode graph builder binray operators.
4. Updates unit tests of bytecode graph builder for unary operators to match
control and effect.

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

to

==========
[Interpreter] Add support for cast operators to bytecode graph builder and
an optomization to remove redundant cast operations.

1. Adds an optimization to remove redundant ToBoolean and ToName operations.
2. Adds implementation and tests for cast operatorts to bytecode graph builder.

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

[mythria, 2015-11-30 12:34:26]

Orion,
I rebased the patch and removed NodeMatchers for binary operators and type
conversion operators from test/unititests/compiler/node-test-utils.cc since we
no longer need them. Can you please take a look? 

I am sorry the changes related to rebase and changes to this cl are merged into
one. I did not realize this when rebasing. I marked the regions that have
changed due to rebase. Hope it is a bit better :)

https://codereview.chromium.org/1468003002/diff/80001/test/unittests/compiler...
File test/unittests/compiler/node-test-utils.cc (right):

https://codereview.chromium.org/1468003002/diff/80001/test/unittests/compiler...
test/unittests/compiler/node-test-utils.cc:1409: class IsJSBinopMatcher final :
public NodeMatcher {
I removed this part.

https://codereview.chromium.org/1468003002/diff/80001/test/unittests/compiler...
test/unittests/compiler/node-test-utils.cc:1545: 
Part of rebase

https://codereview.chromium.org/1468003002/diff/80001/test/unittests/compiler...
test/unittests/compiler/node-test-utils.cc:2550: Matcher<Node*>
IsJSDeleteProperty(const Matcher<Node*>& object_value_matcher,
Part of rebase

https://codereview.chromium.org/1468003002/diff/80001/test/unittests/compiler...
test/unittests/compiler/node-test-utils.cc:2654: Matcher<Node*>
IsJSBinaryOperation(IrOpcode::Value opcode,
I removed this

https://codereview.chromium.org/1468003002/diff/80001/test/unittests/compiler...
test/unittests/compiler/node-test-utils.cc:2688: IS_BINOP_MATCHER(Word64Shr)
Part of rebase

https://codereview.chromium.org/1468003002/diff/80001/test/unittests/compiler...
test/unittests/compiler/node-test-utils.cc:2743: 
I removed this.

[oth, 2015-11-30 13:39:54]

Thanks, looks fine. lgtm.

[mythria, 2015-11-30 13:41:40]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-11-30 13:41:40]

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/1468003002/#ps100001
(title: "rebased the patch and removed unused definitions of matcher classes in
unittests")

[commit-bot: I haz the power, 2015-11-30 13:41:52]

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

[commit-bot: I haz the power, 2015-11-30 13:50:00]

Description was changed from

==========
[Interpreter] Add support for cast operators to bytecode graph builder and
an optomization to remove redundant cast operations.

1. Adds an optimization to remove redundant ToBoolean and ToName operations.
2. Adds implementation and tests for cast operatorts to bytecode graph builder.

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

to

==========
[Interpreter] Add support for cast operators to bytecode graph builder and
an optomization to remove redundant cast operations.

1. Adds an optimization to remove redundant ToBoolean and ToName operations.
2. Adds implementation and tests for cast operatorts to bytecode graph builder.

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

[commit-bot: I haz the power, 2015-11-30 13:50:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-30 13:50:23]

Description was changed from

==========
[Interpreter] Add support for cast operators to bytecode graph builder and
an optomization to remove redundant cast operations.

1. Adds an optimization to remove redundant ToBoolean and ToName operations.
2. Adds implementation and tests for cast operatorts to bytecode graph builder.

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

to

==========
[Interpreter] Add support for cast operators to bytecode graph builder and
an optomization to remove redundant cast operations.

1. Adds an optimization to remove redundant ToBoolean and ToName operations.
2. Adds implementation and tests for cast operatorts to bytecode graph builder.

BUG=v8:4280
LOG=N

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

[commit-bot: I haz the power, 2015-11-30 13:50:24]

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

[rmcilroy, 2015-12-07 11:56:02]

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

[rmcilroy, 2015-12-07 11:56:03]

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

https://codereview.chromium.org/1468003002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:20: return Bytecode::kLast;
Bytecode::kLast is the same as the last valid bytecode declared in bytecodes.h -
i.e., Bytecode::kReturn right now. Are you sure you want to return this as a
'none' value? It seems like it could cause bugs or confusion if you want to do a
test against kReturn or if the bytecode declaration order changes such that
kLast is equal to one of the bytecodes being checked below.

https://codereview.chromium.org/1468003002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:616: switch
(previous_bytecode.GetBytecode()) {
Optional nit - It would be nice to have some tests of these cases below in
test-bytecode-generator.