[Interpreter] Add interpreter support for compare ops and ToBoolean.

[Interpreter] Add interpreter support for compare ops and ToBoolean.

The comparison operators and ToBoolean are implemented by calling into
the runtime. There are new runtime methods are prefixed with Interpreter
to make use case clear.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/17363fa4f3bbf483ff68368c7b01355adfdce72f
Cr-Commit-Position: refs/heads/master@{#30983}

[oth, 2015-09-28 08:41:59]

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

[oth, 2015-09-28 08:42:00]

bmeurer@chromium.org: Please review changes in the runtime.

rmcilroy@chromium.org: Please review changes.

Thanks

[Benedikt Meurer, 2015-09-28 09:05:22]

bmeurer@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Benedikt Meurer, 2015-09-28 09:05:23]

https://codereview.chromium.org/1369123002/diff/40001/src/runtime/runtime-int...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1369123002/diff/40001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:14: static Object* ComparisonHelper(Isolate*
isolate, ComparisonResult expected,
Please don't write your own wrapper, but use Object::LessThan and friends, which
do the right thing.

https://codereview.chromium.org/1369123002/diff/40001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:33: return ComparisonHelper(isolate,
ComparisonResult::kEqual, x, y);
Equals and NotEquals cannot use Compare; you need to use Object::Equals instead
(which implements abstract equality).

[Benedikt Meurer, 2015-09-28 10:19:53]

https://codereview.chromium.org/1369123002/diff/60001/src/runtime/runtime-int...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1369123002/diff/60001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:30: return MaybeBooleanHelper(isolate,
Object::Equals(x, y));
Please avoid this indirection and the bit flipping. Just use the straightline
code:

Maybe<bool> result = Object::Equals(x, y);
if (!result.IsJust()) return isolate->heap()->exception();
return isolate->heap()->ToBoolean(result.FromJust());

[oth, 2015-09-28 10:23:02]

Thanks Benedikt, kindly appreciated.

https://codereview.chromium.org/1369123002/diff/40001/src/runtime/runtime-int...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1369123002/diff/40001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:14: static Object* ComparisonHelper(Isolate*
isolate, ComparisonResult expected,
On 2015/09/28 09:05:23, Benedikt Meurer wrote:
> Please don't write your own wrapper, but use Object::LessThan and friends,
which
> do the right thing.

Done.

https://codereview.chromium.org/1369123002/diff/40001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:33: return ComparisonHelper(isolate,
ComparisonResult::kEqual, x, y);
On 2015/09/28 09:05:23, Benedikt Meurer wrote:
> Equals and NotEquals cannot use Compare; you need to use Object::Equals
instead
> (which implements abstract equality).

Done.

[Benedikt Meurer, 2015-09-28 10:24:38]

https://codereview.chromium.org/1369123002/diff/80001/src/runtime/runtime-int...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1369123002/diff/80001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:29: return MaybeBooleanHelper(isolate,
Object::Equals(x, y));
I'd really prefer to code it directly without the Helper. It's just three lines
of code and it's more explicit than the helper indirection IMHO.

[oth, 2015-09-28 10:38:45]

Thanks, feedback incorporated.

https://codereview.chromium.org/1369123002/diff/80001/src/runtime/runtime-int...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1369123002/diff/80001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:29: return MaybeBooleanHelper(isolate,
Object::Equals(x, y));
On 2015/09/28 10:24:38, Benedikt Meurer wrote:
> I'd really prefer to code it directly without the Helper. It's just three
lines
> of code and it's more explicit than the helper indirection IMHO.

Done.

[Michael Starzinger, 2015-09-28 11:53:14]

https://codereview.chromium.org/1369123002/diff/100001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/1369123002/diff/100001/src/runtime/runtime.h#...
src/runtime/runtime.h:219: #define FOR_EACH_INTRINSIC_FOR_INTERPRETER(F) \
nit: s/FOR_EACH_INTRINSIC_FOR_INTERPRETER/FOR_EACH_INTRINSIC_INTERPRETER/ for
consistency here.

https://codereview.chromium.org/1369123002/diff/100001/test/unittests/runtime...
File test/unittests/runtime/runtime-interpreter-unittest.cc (right):

https://codereview.chromium.org/1369123002/diff/100001/test/unittests/runtime...
test/unittests/runtime/runtime-interpreter-unittest.cc:17: class
InterpreterRuntimeTest : public TestWithIsolateAndZone {
suggestion: How would you feel about calling this "RuntimeInterpreterTest" for
consistency instead?

[Benedikt Meurer, 2015-09-28 11:54:16]

Ok, LGTM on src/runtime. Didn't check the rest.

[Michael Starzinger, 2015-09-28 12:02:44]

LGTM on compiler and interpreter directory (modulo my nits).

[rmcilroy, 2015-09-28 14:30:39]

lgtm!

https://codereview.chromium.org/1369123002/diff/100001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/1369123002/diff/100001/src/runtime/runtime.h#...
src/runtime/runtime.h:228: F(InterpreterCastToBoolean, 1, 1)
/s/CastToBoolean/ToBoolean for consistency

https://codereview.chromium.org/1369123002/diff/100001/test/cctest/interprete...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1369123002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:855: "if (a == b) { return 1;
}\n"
nit - indent by two spaces - i.e.:
"  if (a..."

https://codereview.chromium.org/1369123002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:866: "} f(1,1);",
nit - space between "1, 1"

https://codereview.chromium.org/1369123002/diff/100001/test/cctest/interprete...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1369123002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:1228: Handle<i::JSObject> object =
factory->NewJSObject(function);
nit - would factory->NewJSArray not do the job?

[oth, 2015-09-28 15:05:38]

Thanks all, all comments incorporated.

https://codereview.chromium.org/1369123002/diff/100001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/1369123002/diff/100001/src/runtime/runtime.h#...
src/runtime/runtime.h:219: #define FOR_EACH_INTRINSIC_FOR_INTERPRETER(F) \
On 2015/09/28 11:53:13, Michael Starzinger wrote:
> nit: s/FOR_EACH_INTRINSIC_FOR_INTERPRETER/FOR_EACH_INTRINSIC_INTERPRETER/ for
> consistency here.

Done.

https://codereview.chromium.org/1369123002/diff/100001/src/runtime/runtime.h#...
src/runtime/runtime.h:228: F(InterpreterCastToBoolean, 1, 1)
On 2015/09/28 14:30:39, rmcilroy wrote:
> /s/CastToBoolean/ToBoolean for consistency

Done.

https://codereview.chromium.org/1369123002/diff/100001/test/cctest/interprete...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1369123002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:855: "if (a == b) { return 1;
}\n"
On 2015/09/28 14:30:39, rmcilroy wrote:
> nit - indent by two spaces - i.e.:
> "  if (a..."

Done.

https://codereview.chromium.org/1369123002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:866: "} f(1,1);",
On 2015/09/28 14:30:39, rmcilroy wrote:
> nit - space between "1, 1"

Done.

https://codereview.chromium.org/1369123002/diff/100001/test/cctest/interprete...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1369123002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:1228: Handle<i::JSObject> object =
factory->NewJSObject(function);
On 2015/09/28 14:30:39, rmcilroy wrote:
> nit - would factory->NewJSArray not do the job?

Done.

https://codereview.chromium.org/1369123002/diff/100001/test/unittests/runtime...
File test/unittests/runtime/runtime-interpreter-unittest.cc (right):

https://codereview.chromium.org/1369123002/diff/100001/test/unittests/runtime...
test/unittests/runtime/runtime-interpreter-unittest.cc:17: class
InterpreterRuntimeTest : public TestWithIsolateAndZone {
On 2015/09/28 11:53:13, Michael Starzinger wrote:
> suggestion: How would you feel about calling this "RuntimeInterpreterTest" for
> consistency instead?

Done. Like it plenty.

[oth, 2015-09-28 15:24:59]

The CQ bit was checked by oth@chromium.org

[oth, 2015-09-28 15:25:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rmcilroy@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1369123002/#ps120001
(title: "Incorporate review feedback from mstarzinger and rmcilroy.")

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

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

[commit-bot: I haz the power, 2015-09-28 18:06:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-09-28 18:07:18]

Patchset 7 (id:??) landed as
https://crrev.com/17363fa4f3bbf483ff68368c7b01355adfdce72f
Cr-Commit-Position: refs/heads/master@{#30983}