[Interpreter] Adds shift operators to interpreter

[Interpreter] Adds shift operators to interpreter

Adds support for following operators
 -Shift left
 -Shift right
 -Shift right logical

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/aacc18f0532676b6fc0f6eec877f01069bb75174
Cr-Commit-Position: refs/heads/master@{#31205}

[mythria, 2015-10-07 13:13:24]

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

[mythria, 2015-10-07 13:13:24]

Hi,

I added support for shift operators to interpreter. Could you please review my
changes.

Thanks and Regards,
Mythri

[oth, 2015-10-07 13:53:17]

Overall looks very good. A couple of minor comments below. Thanks!

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

https://codereview.chromium.org/1392913002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:380: // Left shifts accumulator by the count
specified in register <src>.
Can the comments here and below mention that the accumulator and register will
be cast to Int32 prior to the operation?

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

https://codereview.chromium.org/1392913002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:384: return val >> count;
Can we have a separate test for the shift operators?

The inputs in BinaryOpsSmi don't test the edge cases for shifts. (Shift 0, 31,
32, >32).

[mythria, 2015-10-08 14:33:44]

I added additional tests for shift operators and updated the comments. Please
let me know if they are ok.

Thanks and Regards,
Mythri

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

https://codereview.chromium.org/1392913002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:380: // Left shifts accumulator by the count
specified in register <src>.
On 2015/10/07 13:53:17, oth wrote:
> Can the comments here and below mention that the accumulator and register will
> be cast to Int32 prior to the operation?

Done.

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

https://codereview.chromium.org/1392913002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:384: return val >> count;
On 2015/10/07 13:53:17, oth wrote:
> Can we have a separate test for the shift operators?
> 
> The inputs in BinaryOpsSmi don't test the edge cases for shifts. (Shift 0, 31,
> 32, >32).
> 

Done.

[rmcilroy, 2015-10-08 14:53:07]

Just comment nits, other than that lgtm, thanks!

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

https://codereview.chromium.org/1392913002/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter.cc:380: // Left shifts accumulator by the count
specified in register <src>.
"accumulator" -> "the accumulator"

https://codereview.chromium.org/1392913002/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter.cc:381: // accumulator is converted to a int32 and
register <src> to uint32
nit - a int32 -> an int32

https://codereview.chromium.org/1392913002/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter.cc:381: // accumulator is converted to a int32 and
register <src> to uint32
nit "accumulator" -> "The accumulator" (it's the start of a sentence)

https://codereview.chromium.org/1392913002/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter.cc:382: // before the operation. 5 lsb bits from
register <src> are used as count.
nit - move "." from after "count" to after "0x1F)"

https://codereview.chromium.org/1392913002/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter.cc:392: // Result is sign extended.
Same comments here. Also, either bring the next sentence onto this line, or add
a blank comment line if you want to make a paragraph.

https://codereview.chromium.org/1392913002/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter.cc:404: // Result is zero-filled.
ditto

[oth, 2015-10-08 16:11:38]

lgtm with additional comment on the doc comments. Thanks!

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

https://codereview.chromium.org/1392913002/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter.cc:380: // Left shifts accumulator by the count
specified in register <src>.
Shouldn't it be Left shifts the register <src> by the value specified in the
accumulator? Same with the doc comments for ShiftRight and ShiftRightLogical.

[oth, 2015-10-08 16:14:10]

lgtm with additional comment on the doc comments. Thanks!

[mythria, 2015-10-08 17:34:39]

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

[mythria, 2015-10-08 17:34:39]

I added support for shift operators to the interpreter. Could you please review
the changes.

Thanks and Regards,
Mythri

[Michael Starzinger, 2015-10-12 09:23:48]

LGTM.

[mythria, 2015-10-12 10:23:36]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-10-12 10:23:36]

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/1392913002/#ps100001
(title: "Fixed a test for shift operators and comments")

[commit-bot: I haz the power, 2015-10-12 10:23:52]

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

[commit-bot: I haz the power, 2015-10-12 10:45:27]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-12 10:45:40]

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