[Interpreter] Add bitwise operators (Or, Xor, And) to interpreter

Adds support for following operators
  -Bitwise Or
  -Bitwise Xor
  -Bitwise And

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

[mythria, 2015-10-06 10:58:23]

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

[mythria, 2015-10-06 10:58:24]

Hi,

Could you please review my changes.

Thanks and Regards,
Mythri

[rmcilroy, 2015-10-06 11:09:29]

https://codereview.chromium.org/1386133002/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1386133002/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:64: V(BitwiseAnd, OperandType::kReg8)               
                           \
nit - move these to the bottom of the binary operators section

https://codereview.chromium.org/1386133002/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1386133002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:297: // BitwiseOr <src>
ditto - move these down so they are in the same order as bytecodes are declared.

https://codereview.chromium.org/1386133002/diff/1/test/cctest/interpreter/tes...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1386133002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:807: "function f() { return
%BitwiseOr(1, 2) }\nf()",
This isn't testing your binary operators, it is checking direct calls into the
runtime. What you want is "return 1 | 2" (or you might need to do var a = 1;
return a | 2;" to avoid the operation being constant folded. Also this should go
in TEST(PrimitiveExpressions), not CallRuntime (and it would be great to add a
few more in there for SUB, MOD, MUL, DIV, etc.).

https://codereview.chromium.org/1386133002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:821: "function f() { return
%BitwiseXor(1, 2) }\nf()",
ditto, with '^'.

https://codereview.chromium.org/1386133002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:835: "function f() { return
%BitwiseAnd(1, 2) }\nf()",
ditto with '&'

[rmcilroy, 2015-10-06 11:57:13]

Also please update the description to include "[Interpreter]" in the title, and
to link to BUG=v8:4280. Also I think the grammar should be "Add bitwise
operators (Or, Xor, And)" to interpreter and perhaps add a bit more description
(e.g., https://codereview.chromium.org/1300813005).

[mythria, 2015-10-06 13:47:40]

Hi,

I addressed comments from Ross. Could you review my changes.

Thanks and Regards,
Mythri

https://codereview.chromium.org/1386133002/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1386133002/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:64: V(BitwiseAnd, OperandType::kReg8)               
                           \
On 2015/10/06 11:09:28, rmcilroy wrote:
> nit - move these to the bottom of the binary operators section

Done.

https://codereview.chromium.org/1386133002/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1386133002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:297: // BitwiseOr <src>
On 2015/10/06 11:09:28, rmcilroy wrote:
> ditto - move these down so they are in the same order as bytecodes are
declared.

Done.

https://codereview.chromium.org/1386133002/diff/1/test/cctest/interpreter/tes...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1386133002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:807: "function f() { return
%BitwiseOr(1, 2) }\nf()",
On 2015/10/06 11:09:29, rmcilroy wrote:
> This isn't testing your binary operators, it is checking direct calls into the
> runtime. What you want is "return 1 | 2" (or you might need to do var a = 1;
> return a | 2;" to avoid the operation being constant folded. Also this should
go
> in TEST(PrimitiveExpressions), not CallRuntime (and it would be great to add a
> few more in there for SUB, MOD, MUL, DIV, etc.).

Done.

https://codereview.chromium.org/1386133002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:821: "function f() { return
%BitwiseXor(1, 2) }\nf()",
On 2015/10/06 11:09:29, rmcilroy wrote:
> ditto, with '^'.

Done.

https://codereview.chromium.org/1386133002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:835: "function f() { return
%BitwiseAnd(1, 2) }\nf()",
On 2015/10/06 11:09:28, rmcilroy wrote:
> ditto with '&'

Done.

[rmcilroy, 2015-10-06 13:52:03]

Thanks! One fix in description (BYteCodeArrayBuilder -> BytecodeArrayBuilder),
but otherwise lgtm.

[oth, 2015-10-08 09:14:57]

lgtm

[oth, 2015-10-08 09:14:58]

lgtm

[mythria, 2015-10-08 09:26:28]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-10-08 09:26:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1386133002/#ps60001
(title: "rebased the patch")

[commit-bot: I haz the power, 2015-10-08 09:26:42]

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

[commit-bot: I haz the power, 2015-10-08 09:27:26]

The CQ bit was unchecked by commit-bot@chromium.org

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

Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[rmcilroy, 2015-10-08 10:02:06]

You will also need to get someone to stamp with ownership of src/compiler
(mstarzinger@ would be an option).

[mythria, 2015-10-08 14:23:51]

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

[mythria, 2015-10-08 14:23:52]

I added support for bitwise operators to interpreter. Could you please review
the changes especially in the file src/compiler/bytecode-graph-builder.cc and
let me know if they are ok. 

Thanks and Regards,
Mythri

[Michael Starzinger, 2015-10-12 09:24:58]

LGTM.

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

The CQ bit was checked by mythria@chromium.org

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

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/1386133002/#ps80001
(title: "rebased the patch")

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

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

[commit-bot: I haz the power, 2015-10-12 10:46:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-12 10:46:32]

Failed to apply patch for src/interpreter/bytecode-array-builder.cc:
While running git apply --index -3 -p1;
  error: patch failed: src/interpreter/bytecode-array-builder.cc:604
  Falling back to three-way merge...
  Applied patch to 'src/interpreter/bytecode-array-builder.cc' with conflicts.
  U src/interpreter/bytecode-array-builder.cc

Patch:       src/interpreter/bytecode-array-builder.cc
Index: src/interpreter/bytecode-array-builder.cc
diff --git a/src/interpreter/bytecode-array-builder.cc
b/src/interpreter/bytecode-array-builder.cc
index
1ce19f901f4ed46c0474f538e9c3e5debf9ad330..cb8fa68cff09bcca50675f007511c359dd849e3d
100644
--- a/src/interpreter/bytecode-array-builder.cc
+++ b/src/interpreter/bytecode-array-builder.cc
@@ -604,6 +604,12 @@ Bytecode
BytecodeArrayBuilder::BytecodeForBinaryOperation(Token::Value op) {
       return Bytecode::kDiv;
     case Token::Value::MOD:
       return Bytecode::kMod;
+    case Token::Value::BIT_OR:
+      return Bytecode::kBitwiseOr;
+    case Token::Value::BIT_XOR:
+      return Bytecode::kBitwiseXor;
+    case Token::Value::BIT_AND:
+      return Bytecode::kBitwiseAnd;
     default:
       UNREACHABLE();
       return static_cast<Bytecode>(-1);

[mythria, 2015-10-12 13:34:38]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-10-12 13:34:38]

The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org,
mstarzinger@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1386133002/#ps100001
(title: "rebased the patch")

[commit-bot: I haz the power, 2015-10-12 13:34:47]

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

[commit-bot: I haz the power, 2015-10-12 13:36:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-12 13:36:14]

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