[Interpreter] Merges ToBoolean and JumpIfTrue/False bytecodes

[Interpreter] Merges ToBoolean and JumpIfTrue/False bytecodes

Adds an optimization to emit JumpIfToBooleanTrue/False instead
of ToBoolean followed by JumpIfTrue/False if the value in the
accumulator is not boolean.

BUG=v8:4280
LOG=N

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

[mythria, 2015-10-28 14:20:59]

Description was changed from

==========
Merges ToBoolean and JumpIfTrue/False bytecodes

Adds an optimization to emit JumpIfToBooleanTrue/False instead
of ToBoolean followed by JumpIfTrue/False if the value in the
accumulator is not boolean.

BUG=
==========

to

==========
[Interpreter] Merges ToBoolean and JumpIfTrue/False bytecodes

Adds an optimization to emit JumpIfToBooleanTrue/False instead
of ToBoolean followed by JumpIfTrue/False if the value in the
accumulator is not boolean.

BUG=
==========

[mythria, 2015-10-28 14:23:09]

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

[mythria, 2015-10-28 14:23:09]

Could you please review the changes.

Thanks and Regards,
Mythri

[rmcilroy, 2015-10-29 10:40:25]

Looks pretty good. 

Please also add:

BUG=v8:4280
LOG=N

to the description (and for all future interpreter CLs).

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

https://codereview.chromium.org/1426913002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:479: return false;
nit - do this as an early out at the to, i.e.: 
if (LastBytecodeInSameBlock()) {
  // If the previous bytecode was from a different block return false.
  return false;
}

switch....

https://codereview.chromium.org/1426913002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:610:
GetJumpWithToBoolean(jump_bytecode, is_cast_required);
Avoid the need for the is_cast_required argument and extra variable by just
doing:

if (!IsAccumulatorBoolean()) {
  jump_bytecode = GetJumpWithToBoolean(jump_bytecode);
}

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

https://codereview.chromium.org/1426913002/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:374: // ignore
compare operation
nit - capital letter and full stop (throughout)

[mythria, 2015-10-29 17:24:27]

Description was changed from

==========
[Interpreter] Merges ToBoolean and JumpIfTrue/False bytecodes

Adds an optimization to emit JumpIfToBooleanTrue/False instead
of ToBoolean followed by JumpIfTrue/False if the value in the
accumulator is not boolean.

BUG=
==========

to

==========
[Interpreter] Merges ToBoolean and JumpIfTrue/False bytecodes

Adds an optimization to emit JumpIfToBooleanTrue/False instead
of ToBoolean followed by JumpIfTrue/False if the value in the
accumulator is not boolean.

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

[mythria, 2015-10-29 17:58:39]

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

[mythria, 2015-10-29 17:58:39]

Addressed comments from Ross. Could you review the changes.

Thanks and Regards,
Mythri

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

https://codereview.chromium.org/1426913002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:479: return false;
On 2015/10/29 10:40:24, rmcilroy wrote:
> nit - do this as an early out at the to, i.e.: 
> if (LastBytecodeInSameBlock()) {
>   // If the previous bytecode was from a different block return false.
>   return false;
> }
> 
> switch....

Done.

https://codereview.chromium.org/1426913002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:610:
GetJumpWithToBoolean(jump_bytecode, is_cast_required);
On 2015/10/29 10:40:24, rmcilroy wrote:
> Avoid the need for the is_cast_required argument and extra variable by just
> doing:
> 
> if (!IsAccumulatorBoolean()) {
>   jump_bytecode = GetJumpWithToBoolean(jump_bytecode);
> }

Done.

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

https://codereview.chromium.org/1426913002/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:374: // ignore
compare operation
On 2015/10/29 10:40:24, rmcilroy wrote:
> nit - capital letter and full stop (throughout)

Done.

[rmcilroy, 2015-10-30 10:36:00]

LGTM with comments (but please wait for Orion to review too).

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

https://codereview.chromium.org/1426913002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:460: // the accumulator return true.
nit - put comment on one line (or as much of it as will fit on the line above).

https://codereview.chromium.org/1426913002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:597: }
You probably need to return return static_cast<Bytecode>(-1); at the bottom here
otherwise gcc (on the bots) will complain.

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

https://codereview.chromium.org/1426913002/diff/40001/test/unittests/interpre...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:445:
Bytecodes::ToByte(Bytecode::kReturn));
I'm not convinced this check is necessary (given it's implicitly checked above),
however if you want to keep it could you pull some of the logic out of the CHECK
to make it more managable - e.g., pulling out:

Handle<Object> jump_offset = iterator.GetConstantForIndexOperand(0);
CHECK_EQ(*jump_offset, Smi::FromInt(kFarJumpDistance - 12));
CHECK_EQ(array->get(iterator.current_offset(),
Smi::cast(*jump_offset->value())), 
         Bytecodes::ToByte(Bytecode::kReturn));

https://codereview.chromium.org/1426913002/diff/40001/test/unittests/interpre...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:457:
Smi::cast(*iterator.GetConstantForIndexOperand(0))->value()),
ditto

[oth, 2015-10-30 10:46:32]

lgtm. A minor comment about naming, but super.

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

https://codereview.chromium.org/1426913002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:237: bool IsAccumulatorBoolean();
For readability I'd suggest changing the name to NeedToBooleanCast() and
negating sense of output. The code currently uses !IsAccumulatorBoolean().

[rmcilroy, 2015-10-30 10:50:07]

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

https://codereview.chromium.org/1426913002/diff/40001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:499: X X X X X X X X X X X X
X X X X X X X X X X X X X X X X X X X X X
Now that https://codereview.chromium.org/1413863010/ is landing, could you
update this to use REPEAT_32(...) or similar instead of defining 'X'.

[mythria, 2015-10-30 11:44:47]

Thanks for reviews. I addressed all comments.

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

https://codereview.chromium.org/1426913002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:460: // the accumulator return true.
On 2015/10/30 10:36:00, rmcilroy wrote:
> nit - put comment on one line (or as much of it as will fit on the line
above).

Done.

https://codereview.chromium.org/1426913002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:597: }
On 2015/10/30 10:36:00, rmcilroy wrote:
> You probably need to return return static_cast<Bytecode>(-1); at the bottom
here
> otherwise gcc (on the bots) will complain. 

Done.

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

https://codereview.chromium.org/1426913002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:237: bool IsAccumulatorBoolean();
On 2015/10/30 10:46:32, oth wrote:
> For readability I'd suggest changing the name to NeedToBooleanCast() and
> negating sense of output. The code currently uses !IsAccumulatorBoolean().
> 

Done.

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

https://codereview.chromium.org/1426913002/diff/40001/test/unittests/interpre...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:445:
Bytecodes::ToByte(Bytecode::kReturn));
On 2015/10/30 10:36:00, rmcilroy wrote:
> I'm not convinced this check is necessary (given it's implicitly checked
above),
> however if you want to keep it could you pull some of the logic out of the
CHECK
> to make it more managable - e.g., pulling out:
> 
> Handle<Object> jump_offset = iterator.GetConstantForIndexOperand(0);
> CHECK_EQ(*jump_offset, Smi::FromInt(kFarJumpDistance - 12));
> CHECK_EQ(array->get(iterator.current_offset(),
> Smi::cast(*jump_offset->value())), 
>          Bytecodes::ToByte(Bytecode::kReturn));

Done.

https://codereview.chromium.org/1426913002/diff/40001/test/unittests/interpre...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:457:
Smi::cast(*iterator.GetConstantForIndexOperand(0))->value()),
On 2015/10/30 10:36:00, rmcilroy wrote:
> ditto

Done.

[rmcilroy, 2015-10-30 12:09:53]

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

https://codereview.chromium.org/1426913002/diff/40001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:499: X X X X X X X X X X X X
X X X X X X X X X X X X X X X X X X X X X
On 2015/10/30 10:50:06, rmcilroy wrote:
> Now that https://codereview.chromium.org/1413863010/ is landing, could you
> update this to use REPEAT_32(...) or similar instead of defining 'X'.

Still this please.

[mythria, 2015-10-30 14:19:08]

Along with rebasing the patch, I did following changes:

1. fixed test-bytecode-generator to use the new Repeat_32 macro along with
rebase. 
2. Added ForInDone to the list of operators that generate boolean value.

Please let me know if it is Ok.

Thanks and Regards,
Mythri

[rmcilroy, 2015-10-30 14:42:52]

> 1. fixed test-bytecode-generator to use the new Repeat_32 macro along with
> rebase. 

Great thanks!

> 2. Added ForInDone to the list of operators that generate boolean value.

I had a quick look and this seems valid - Orion to confirm.

[mythria, 2015-10-30 16:47:01]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-10-30 16:47:01]

The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1426913002/#ps80001
(title: "rebased the patch and used the new Repeat_32 macro for tests")

[commit-bot: I haz the power, 2015-10-30 16:47:13]

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

[commit-bot: I haz the power, 2015-10-30 16:48:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-30 16:48:43]

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