[Interpreter] TurboFan implementation of intrinsics.

[Interpreter] TurboFan implementation of intrinsics.

Introduces a bytecode whose handler executes the equivalent of %_IsArray and %_IsJSReceiver without a runtime call.

BUG=v8:4822
LOG=y

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

[oth, 2016-02-01 09:52:33]

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

[oth, 2016-02-01 09:52:33]

Cool, thanks for sharing. A few comments below, but think this is a useful
direction.

Tests in test-interpreter and test-run-bytecode-graph-builder would be handy to
have.

Thanks

https://codereview.chromium.org/1645763003/diff/1/src/compiler/interpreter-as...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1645763003/diff/1/src/compiler/interpreter-as...
src/compiler/interpreter-assembler.cc:719: /*  return_values[2] =
Dead code.

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

https://codereview.chromium.org/1645763003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:1140: // Arg is in the accumulator
Comment seems distracting. Neither the interpreter or compiler implementations
look at the accumulator.

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

https://codereview.chromium.org/1645763003/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:267: V(IntrinsicOneArg, OperandType::kIdx16,
OperandType::kMaybeReg16)
Can this be called this CallIntrinsic or InvokeIntrinsic?

Is the final register operand optional?

If not it should be a kReg8 operand here. Register translation will ensure a
single register operand is always reachable. So far kMaybeRegXX operands are
used for variable register arguments where it's valid to have a zero count of
operands.

If we foresee supporting other intrinsics in future with zero or >= two
arguments, then we could consider rationalizing into two bytecodes for all
cases:
CallIntrinsic OperandType::kIdx16, OperandType::kMaybeReg8,
OperandType::kRegCount8
CallIntrinsicWide OperandType::kIdx16, OperandType::kMaybeReg16,
OperandType::kRegCount16

[rmcilroy, 2016-02-01 11:04:44]

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

[rmcilroy, 2016-02-01 11:04:44]

Cool stuff. I have overarching thoughts on the design. I'm happy to chat about
these in person (either grab me on a GVC or I we can chat about it in the
Interpreter meeting next Wednesday), but the overall thoughts are:

 - The meat of the intrinsic implementation shouldn't live in
InterpreterAssembler, instead that should just expose the basic building blocks
required (e.g., branches, labels, etc.).
 - Given that there will probably be a few of these, it probably makes sense
that their implementation lives in a separate file (e.g,
interpreter-intrinsics.cc).
 - I think it would make sense to have an intrinsic dispatch table - i.e., each
intrinsic would be compiled to it's own code object, and the entry points
inserted into an intrinsic dispatch table, which is used by the InvokeIntrinsic
bytecode to dispatch to the intrinsic implementation (much like how the
bytecodes are dispatched). The end of the intrinsic could dispatch directly to
the next bytecode.
 - To do this we would need to give the intrinsic's their own id (which would be
a smaller range than the full RuntimeId enum), and convert back from this to
RuntimeId in the bytecode graph builder.

Feel free to grab me if you want to discuss any of this in-person.

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

https://codereview.chromium.org/1645763003/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:267: V(IntrinsicOneArg, OperandType::kIdx16,
OperandType::kMaybeReg16)
On 2016/02/01 09:52:33, oth wrote:
> Can this be called this CallIntrinsic or InvokeIntrinsic?
> 
> Is the final register operand optional?
> 
> If not it should be a kReg8 operand here. Register translation will ensure a
> single register operand is always reachable. So far kMaybeRegXX operands are
> used for variable register arguments where it's valid to have a zero count of
> operands.
> 
> If we foresee supporting other intrinsics in future with zero or >= two
> arguments, then we could consider rationalizing into two bytecodes for all
> cases:
> CallIntrinsic OperandType::kIdx16, OperandType::kMaybeReg8,
> OperandType::kRegCount8
> CallIntrinsicWide OperandType::kIdx16, OperandType::kMaybeReg16,
> OperandType::kRegCount16

+1 for InvokeIntrinsic and the comment on using kReg8 instead of kMaybeReg8.

[epertoso, 2016-03-03 11:18:19]

epertoso@chromium.org changed reviewers:
+ titzer@chromium.org

[epertoso, 2016-03-03 11:19:52]

On 2016/02/01 at 11:04:44, rmcilroy wrote:
> Cool stuff. I have overarching thoughts on the design. I'm happy to chat about
these in person (either grab me on a GVC or I we can chat about it in the
Interpreter meeting next Wednesday), but the overall thoughts are:
> 
>  - The meat of the intrinsic implementation shouldn't live in
InterpreterAssembler, instead that should just expose the basic building blocks
required (e.g., branches, labels, etc.).
>  - Given that there will probably be a few of these, it probably makes sense
that their implementation lives in a separate file (e.g,
interpreter-intrinsics.cc).
>  - I think it would make sense to have an intrinsic dispatch table - i.e.,
each intrinsic would be compiled to it's own code object, and the entry points
inserted into an intrinsic dispatch table, which is used by the InvokeIntrinsic
bytecode to dispatch to the intrinsic implementation (much like how the
bytecodes are dispatched). The end of the intrinsic could dispatch directly to
the next bytecode.
>  - To do this we would need to give the intrinsic's their own id (which would
be a smaller range than the full RuntimeId enum), and convert back from this to
RuntimeId in the bytecode graph builder.
> 
> Feel free to grab me if you want to discuss any of this in-person.
> 
> https://codereview.chromium.org/1645763003/diff/1/src/interpreter/bytecodes.h
> File src/interpreter/bytecodes.h (right):
> 
>
https://codereview.chromium.org/1645763003/diff/1/src/interpreter/bytecodes.h...
> src/interpreter/bytecodes.h:267: V(IntrinsicOneArg, OperandType::kIdx16,
OperandType::kMaybeReg16)
> On 2016/02/01 09:52:33, oth wrote:
> > Can this be called this CallIntrinsic or InvokeIntrinsic?
> > 
> > Is the final register operand optional?
> > 
> > If not it should be a kReg8 operand here. Register translation will ensure a
> > single register operand is always reachable. So far kMaybeRegXX operands are
> > used for variable register arguments where it's valid to have a zero count
of
> > operands.
> > 
> > If we foresee supporting other intrinsics in future with zero or >= two
> > arguments, then we could consider rationalizing into two bytecodes for all
> > cases:
> > CallIntrinsic OperandType::kIdx16, OperandType::kMaybeReg8,
> > OperandType::kRegCount8
> > CallIntrinsicWide OperandType::kIdx16, OperandType::kMaybeReg16,
> > OperandType::kRegCount16
> 
> +1 for InvokeIntrinsic and the comment on using kReg8 instead of kMaybeReg8.

I've addressed some of the issues. I still use Runtime::FunctionID instead of
introducing another enum.

PTAL

[epertoso, 2016-03-03 11:20:40]

https://codereview.chromium.org/1645763003/diff/1/src/compiler/interpreter-as...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1645763003/diff/1/src/compiler/interpreter-as...
src/compiler/interpreter-assembler.cc:719: /*  return_values[2] =
On 2016/02/01 at 09:52:33, oth wrote:
> Dead code.

Removed.

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

https://codereview.chromium.org/1645763003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:1140: // Arg is in the accumulator
On 2016/02/01 at 09:52:33, oth wrote:
> Comment seems distracting. Neither the interpreter or compiler implementations
look at the accumulator.

Removed.

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

https://codereview.chromium.org/1645763003/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:267: V(IntrinsicOneArg, OperandType::kIdx16,
OperandType::kMaybeReg16)
On 2016/02/01 at 09:52:33, oth wrote:
> Can this be called this CallIntrinsic or InvokeIntrinsic?
> 
> Is the final register operand optional?
> 
> If not it should be a kReg8 operand here. Register translation will ensure a
single register operand is always reachable. So far kMaybeRegXX operands are
used for variable register arguments where it's valid to have a zero count of
operands.
> 
> If we foresee supporting other intrinsics in future with zero or >= two
arguments, then we could consider rationalizing into two bytecodes for all
cases:
> CallIntrinsic OperandType::kIdx16, OperandType::kMaybeReg8,
OperandType::kRegCount8
> CallIntrinsicWide OperandType::kIdx16, OperandType::kMaybeReg16,
OperandType::kRegCount16

Added InvokeIntrinsic and InvokeIntrinsicWide. As we might be adding support for
other intrinsics, I used kMaybeRegXX and kRegCountXX.

[rmcilroy, 2016-03-05 04:21:46]

Sorry for the delay, I've been traveling. Overall looking good - thanks for
addressing the things we discussed. I have a couple of fairly minor comments.
Also, could you create a tracking bug with the Hotlist-Ignition label and add it
to the description?

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

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1127: Bytecode bytecode =
IntrinsicsHelper::IsSupported(function_id)
Please pull this out above the if block and then add entries for
kInvokeIntrinsic and kCallRuntime to BytecodeForWideOperands.

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter-intrinsics.cc (right):

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:27: Node*
IntrinsicsHelper::DispatchIntrinsic(Node* function_id, Node* context,
nit - I would prefer InvokeIntrinsic, dispatch is a bit overloaded in the
interpreter.

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:50: result.Bind(name(first_arg_reg)); 
 \
Could you add a check (guarded with FLAG_debug_code) which checks the number of
arguments is correct? You probably want to have a arg count in the
INTRINSICS_LIST for future intrinsics which take different numbers of arguments
in order to pull out the correct arguments here to pass to the intrinsic
generation functions.

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:68: Node* arg =
assembler_->LoadRegister(input);
Nit - How about #defining __ to assembler_-> like interpreter.cc to make this
code a bit less cluttered?

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:70:
assembler_->Branch(assembler_->WordIsSmi(arg), &if_smi, &if_notsmi);
/s/if_notsmi/if_not_smi/

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:84: if_isnotreceiver(assembler_);
nit - if_receiever and if_not_receiver

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:107:
assembler_->Branch(assembler_->WordIsSmi(arg), &if_smi, &if_notsmi);
ditto

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:125:
assembler_->Int32Constant(JS_ARRAY_TYPE)),
Could we share the code between IsJSReciever and IsArray in a common helper? The
only difference seems to be the instance type to compare.

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

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1921: void
Interpreter::DoInvokeIntrinsic(InterpreterAssembler* assembler) {
nit - could you move these below CallRuntime

[epertoso, 2016-03-07 11:26:36]

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

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1127: Bytecode bytecode =
IntrinsicsHelper::IsSupported(function_id)
On 2016/03/05 at 04:21:46, rmcilroy (Slow - Traveling) wrote:
> Please pull this out above the if block and then add entries for
kInvokeIntrinsic and kCallRuntime to BytecodeForWideOperands.

Done.

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter-intrinsics.cc (right):

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:27: Node*
IntrinsicsHelper::DispatchIntrinsic(Node* function_id, Node* context,
On 2016/03/05 at 04:21:46, rmcilroy (Slow - Traveling) wrote:
> nit - I would prefer InvokeIntrinsic, dispatch is a bit overloaded in the
interpreter.

Done.

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:50: result.Bind(name(first_arg_reg)); 
 \
On 2016/03/05 at 04:21:46, rmcilroy (Slow - Traveling) wrote:
> Could you add a check (guarded with FLAG_debug_code) which checks the number
of arguments is correct? You probably want to have a arg count in the
INTRINSICS_LIST for future intrinsics which take different numbers of arguments
in order to pull out the correct arguments here to pass to the intrinsic
generation functions.

Done.

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:68: Node* arg =
assembler_->LoadRegister(input);
On 2016/03/05 at 04:21:46, rmcilroy (Slow - Traveling) wrote:
> Nit - How about #defining __ to assembler_-> like interpreter.cc to make this
> code a bit less cluttered?

Done.

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:70:
assembler_->Branch(assembler_->WordIsSmi(arg), &if_smi, &if_notsmi);
On 2016/03/05 at 04:21:46, rmcilroy (Slow - Traveling) wrote:
> /s/if_notsmi/if_not_smi/

Done.

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:84: if_isnotreceiver(assembler_);
On 2016/03/05 at 04:21:46, rmcilroy (Slow - Traveling) wrote:
> nit - if_receiever and if_not_receiver

Done.

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:107:
assembler_->Branch(assembler_->WordIsSmi(arg), &if_smi, &if_notsmi);
On 2016/03/05 at 04:21:46, rmcilroy (Slow - Traveling) wrote:
> ditto

Done.

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:125:
assembler_->Int32Constant(JS_ARRAY_TYPE)),
On 2016/03/05 at 04:21:46, rmcilroy (Slow - Traveling) wrote:
> Could we share the code between IsJSReciever and IsArray in a common helper?
The only difference seems to be the instance type to compare.

Done. 

The difference is also the type of comparison ('==' vs '>='), but I've extracted
a common helper anyway as other intrinsic we might implement will benefit from
it.

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

https://codereview.chromium.org/1645763003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1921: void
Interpreter::DoInvokeIntrinsic(InterpreterAssembler* assembler) {
On 2016/03/05 at 04:21:46, rmcilroy (Slow - Traveling) wrote:
> nit - could you move these below CallRuntime

Done. I've also added a comment.

[epertoso, 2016-03-07 11:48:41]

Description was changed from

==========
Prototype: TurboFan implementation of intrinsics.

Introduces a bytecode whose handler executes the equivalent of %_IsArray and
%_IsJSReceiver without a runtime call.

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

to

==========
[Interpreter] TurboFan implementation of intrinsics.

Introduces a bytecode whose handler executes the equivalent of %_IsArray and
%_IsJSReceiver without a runtime call.

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

[rmcilroy, 2016-03-08 11:19:31]

Thanks, LGTM, but please file a tracking bug and add it to the description.

https://codereview.chromium.org/1645763003/diff/60001/src/bailout-reason.h
File src/bailout-reason.h (right):

https://codereview.chromium.org/1645763003/diff/60001/src/bailout-reason.h#ne...
src/bailout-reason.h:268: "Wrong number of argument for intrinsic")             
                    \
/a/argument/arguments

https://codereview.chromium.org/1645763003/diff/60001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1645763003/diff/60001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1431: void
BytecodeGraphBuilder::BuildInvokeIntrinsic() {
nit - move up below VisitCallRuntimeForPairWide

https://codereview.chromium.org/1645763003/diff/60001/src/interpreter/interpr...
File src/interpreter/interpreter-intrinsics.cc (right):

https://codereview.chromium.org/1645763003/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:22: return true;
nit - indent

https://codereview.chromium.org/1645763003/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:53: InterpreterAssembler::Label
match(assembler_), mismatch(assembler_); \
nit - could you pull this out into a seperate function (AbortIfArgCountMismatch)

https://codereview.chromium.org/1645763003/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:85: __ Branch(__
Word32Equal(instance_type, __ Int32Constant(type)), &if_true,
just build the condition (i.e., Word32Equal or Int32GreaterThanOrEqual) here and
do the branch outwith the switch please

https://codereview.chromium.org/1645763003/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:106: Node*
IntrinsicsHelper::IsJSReceiver(Node* input) {
These intrinsics are simple enough that it's probably ok, but are we going to
have unittests for the implementations of other intrinsics in IntrinsicsHelper,
or is there other mjsunit / cctest which we can rely on when run with
--ignition?

[epertoso, 2016-03-08 14:11:09]

https://codereview.chromium.org/1645763003/diff/60001/src/bailout-reason.h
File src/bailout-reason.h (right):

https://codereview.chromium.org/1645763003/diff/60001/src/bailout-reason.h#ne...
src/bailout-reason.h:268: "Wrong number of argument for intrinsic")             
                    \
On 2016/03/08 at 11:19:30, rmcilroy wrote:
> /a/argument/arguments

Done.

https://codereview.chromium.org/1645763003/diff/60001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1645763003/diff/60001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1431: void
BytecodeGraphBuilder::BuildInvokeIntrinsic() {
On 2016/03/08 at 11:19:30, rmcilroy wrote:
> nit - move up below VisitCallRuntimeForPairWide

Done.

https://codereview.chromium.org/1645763003/diff/60001/src/interpreter/interpr...
File src/interpreter/interpreter-intrinsics.cc (right):

https://codereview.chromium.org/1645763003/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:22: return true;
On 2016/03/08 at 11:19:30, rmcilroy wrote:
> nit - indent

That's what clang-format does. I can change it manually, but it will fail the
presubmit check.

https://codereview.chromium.org/1645763003/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:53: InterpreterAssembler::Label
match(assembler_), mismatch(assembler_); \
On 2016/03/08 at 11:19:30, rmcilroy wrote:
> nit - could you pull this out into a seperate function
(AbortIfArgCountMismatch)

Sure.

https://codereview.chromium.org/1645763003/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:85: __ Branch(__
Word32Equal(instance_type, __ Int32Constant(type)), &if_true,
On 2016/03/08 at 11:19:30, rmcilroy wrote:
> just build the condition (i.e., Word32Equal or Int32GreaterThanOrEqual) here
and do the branch outwith the switch please

Done.

https://codereview.chromium.org/1645763003/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:106: Node*
IntrinsicsHelper::IsJSReceiver(Node* input) {
On 2016/03/08 at 11:19:30, rmcilroy wrote:
> These intrinsics are simple enough that it's probably ok, but are we going to
have unittests for the implementations of other intrinsics in IntrinsicsHelper,
or is there other mjsunit / cctest which we can rely on when run with
--ignition?

Actually, this is a good point. I'm not sure about the coverage of intrinsics we
have in unit tests, but it would make sense to write some interpreter-specific
test that checks that we don't change the semantics by mistake.

Should I put them in cctest/interpreter/interpreter-test.cc or should I create a
separate file?

[epertoso, 2016-03-08 14:51:48]

Description was changed from

==========
[Interpreter] TurboFan implementation of intrinsics.

Introduces a bytecode whose handler executes the equivalent of %_IsArray and
%_IsJSReceiver without a runtime call.

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

to

==========
[Interpreter] TurboFan implementation of intrinsics.

Introduces a bytecode whose handler executes the equivalent of %_IsArray and
%_IsJSReceiver without a runtime call.

BUG=v8:4822
==========

[rmcilroy, 2016-03-08 16:09:43]

https://codereview.chromium.org/1645763003/diff/60001/src/interpreter/interpr...
File src/interpreter/interpreter-intrinsics.cc (right):

https://codereview.chromium.org/1645763003/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:22: return true;
On 2016/03/08 14:11:09, epertoso wrote:
> On 2016/03/08 at 11:19:30, rmcilroy wrote:
> > nit - indent
> 
> That's what clang-format does. I can change it manually, but it will fail the
> presubmit check.

Ok.

https://codereview.chromium.org/1645763003/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-intrinsics.cc:106: Node*
IntrinsicsHelper::IsJSReceiver(Node* input) {
On 2016/03/08 14:11:09, epertoso wrote:
> On 2016/03/08 at 11:19:30, rmcilroy wrote:
> > These intrinsics are simple enough that it's probably ok, but are we going
to
> have unittests for the implementations of other intrinsics in
IntrinsicsHelper,
> or is there other mjsunit / cctest which we can rely on when run with
> --ignition?
> 
> Actually, this is a good point. I'm not sure about the coverage of intrinsics
we
> have in unit tests, but it would make sense to write some interpreter-specific
> test that checks that we don't change the semantics by mistake.
> 
> Should I put them in cctest/interpreter/interpreter-test.cc or should I create
a
> separate file?

I'd create a separate file cctest/interpreter/interpreter-intrinsics-test.cc.
Hopefully it should be relatively simple to create a test fixture which provides
a callable which runs a given intrinsic and you can call that from the tests.

[epertoso, 2016-03-09 16:34:03]

Description was changed from

==========
[Interpreter] TurboFan implementation of intrinsics.

Introduces a bytecode whose handler executes the equivalent of %_IsArray and
%_IsJSReceiver without a runtime call.

BUG=v8:4822
==========

to

==========
[Interpreter] TurboFan implementation of intrinsics.

Introduces a bytecode whose handler executes the equivalent of %_IsArray and
%_IsJSReceiver without a runtime call.

BUG=v8:4822
LOG=y
==========

[epertoso, 2016-03-10 14:15:48]

Added the tests.

[rmcilroy, 2016-03-10 17:09:35]

Great stuff, lgtm, thanks!

https://codereview.chromium.org/1645763003/diff/120001/test/cctest/interprete...
File test/cctest/interpreter/test-interpreter-intrinsics.cc (right):

https://codereview.chromium.org/1645763003/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter-intrinsics.cc:26: BytecodeArrayBuilder
builder(isolate_, zone_, sizeof...(args), 0, 0);
Maybe add a DCHECK here that IntrinsicsHelper::IsSupported(function_id_);

[epertoso, 2016-03-14 09:58:12]

epertoso@chromium.org changed reviewers:
+ mstarzinger@chromium.org
- titzer@chromium.org

[epertoso, 2016-03-14 09:58:12]

epertoso@chromium.org changed required reviewers:
+ mstarzinger@chromium.org

[epertoso, 2016-03-14 09:58:40]

+mstarzinger for the changes in src/compiler.

https://codereview.chromium.org/1645763003/diff/120001/test/cctest/interprete...
File test/cctest/interpreter/test-interpreter-intrinsics.cc (right):

https://codereview.chromium.org/1645763003/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter-intrinsics.cc:26: BytecodeArrayBuilder
builder(isolate_, zone_, sizeof...(args), 0, 0);
On 2016/03/10 at 17:09:35, rmcilroy wrote:
> Maybe add a DCHECK here that IntrinsicsHelper::IsSupported(function_id_);

I think CHECK makes more sense, especially for people that run the tests in
release first.

[rmcilroy, 2016-03-14 10:14:42]

https://codereview.chromium.org/1645763003/diff/120001/test/cctest/interprete...
File test/cctest/interpreter/test-interpreter-intrinsics.cc (right):

https://codereview.chromium.org/1645763003/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter-intrinsics.cc:26: BytecodeArrayBuilder
builder(isolate_, zone_, sizeof...(args), 0, 0);
On 2016/03/14 09:58:40, epertoso wrote:
> On 2016/03/10 at 17:09:35, rmcilroy wrote:
> > Maybe add a DCHECK here that IntrinsicsHelper::IsSupported(function_id_);
> 
> I think CHECK makes more sense, especially for people that run the tests in
> release first.

Sure, SGTM, thanks.

[Michael Starzinger, 2016-03-14 14:55:56]

LGTM on the compiler piece, didn't look at the rest.

[epertoso, 2016-03-14 14:56:27]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2016-03-14 14:56:27]

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/1645763003/#ps140001
(title: "Update.")

[commit-bot: I haz the power, 2016-03-14 14:56:42]

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

[commit-bot: I haz the power, 2016-03-14 15:02:27]

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

[commit-bot: I haz the power, 2016-03-14 15:02:28]

Try jobs failed on following builders:
  v8_win64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/4424)

[epertoso, 2016-03-14 16:21:21]

The CQ bit was checked by epertoso@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-14 16:21:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1645763003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1645763003/160001

[commit-bot: I haz the power, 2016-03-14 16:53:03]

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

[commit-bot: I haz the power, 2016-03-14 16:53:04]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)

[epertoso, 2016-03-21 15:28:05]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2016-03-21 15:28:05]

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

[commit-bot: I haz the power, 2016-03-21 15:28:20]

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

[epertoso, 2016-03-21 16:33:28]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2016-03-21 16:33:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1645763003/#ps200001
(title: "Fixes the debug code as well.")

[commit-bot: I haz the power, 2016-03-21 16:33:33]

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

[commit-bot: I haz the power, 2016-03-21 17:15:34]

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

[commit-bot: I haz the power, 2016-03-21 17:15:35]

Failed to apply patch for BUILD.gn:
While running git apply --index -3 -p1;
  error: patch failed: BUILD.gn:1133
  Falling back to three-way merge...
  Applied patch to 'BUILD.gn' with conflicts.
  U BUILD.gn

Patch:       BUILD.gn
Index: BUILD.gn
diff --git a/BUILD.gn b/BUILD.gn
index
a756305409d93c203de9b0360f2146205cc8f308..b25a06b50b91349b82e7e0b9e52a02bf19ca98b1
100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -1133,6 +1133,8 @@ source_set("v8_base") {
     "src/interpreter/interpreter.h",
     "src/interpreter/interpreter-assembler.cc",
     "src/interpreter/interpreter-assembler.h",
+    "src/interpreter/interpreter-intrinsics.cc",
+    "src/interpreter/interpreter-intrinsics.h",
     "src/interpreter/register-translator.cc",
     "src/interpreter/register-translator.h",
     "src/interpreter/source-position-table.cc",

[commit-bot: I haz the power, 2016-03-21 17:15:39]

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

[rmcilroy, 2016-03-22 10:58:24]

Still lgtm, thanks.

https://codereview.chromium.org/1645763003/diff/220001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1645763003/diff/220001/src/interpreter/byteco...
src/interpreter/bytecodes.h:226: /* Intrinsics */                               
                          \
nit - newline above (and move near CallRuntime)

[epertoso, 2016-03-22 11:11:19]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2016-03-22 11:11:21]

https://codereview.chromium.org/1645763003/diff/220001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1645763003/diff/220001/src/interpreter/byteco...
src/interpreter/bytecodes.h:226: /* Intrinsics */                               
                          \
On 2016/03/22 at 10:58:24, rmcilroy wrote:
> nit - newline above (and move near CallRuntime)

Done.

[epertoso, 2016-03-22 11:11:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1645763003/#ps240001
(title: "Merged with oth@'s changes.")

[commit-bot: I haz the power, 2016-03-22 11:11:28]

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

[commit-bot: I haz the power, 2016-03-22 11:35:16]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-03-22 11:36:15]

Description was changed from

==========
[Interpreter] TurboFan implementation of intrinsics.

Introduces a bytecode whose handler executes the equivalent of %_IsArray and
%_IsJSReceiver without a runtime call.

BUG=v8:4822
LOG=y
==========

to

==========
[Interpreter] TurboFan implementation of intrinsics.

Introduces a bytecode whose handler executes the equivalent of %_IsArray and
%_IsJSReceiver without a runtime call.

BUG=v8:4822
LOG=y

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

[commit-bot: I haz the power, 2016-03-22 11:36:17]

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